Re: [PATCH v14 7/9] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection

[Chaoyi Chen]

Hi Luca,

On 3/3/2026 1:49 AM, Luca Ceresoli wrote:
> On Mon Jan 19, 2026 at 8:30 AM CET, Chaoyi Chen wrote:
>> From: Chaoyi Chen <chaoyi.chen@xxxxxxxxxxxxxx>
>>
>> The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And
>> the CDN-DP can be switched to output to one of the PHYs. If both ports
>> are plugged into DP, DP will select the first port for output.
>>
>> This patch adds support for multiple bridges, enabling users to flexibly
>> select the output port. For each PHY port, a separate encoder and bridge
>> are registered.
>>
>> The change is based on the DRM AUX HPD bridge, rather than the
>> extcon approach. This requires the DT to correctly describe the
>> connections between the first bridge in bridge chain and DP
>> controller. For example, the bridge chain may be like this:
>>
>> PHY aux birdge -> fsa4480 analog audio switch bridge ->
>> onnn,nb7vpq904m USB reminder bridge -> USB-C controller AUX HPD bridge
>>
>> In this case, the connection relationships among the PHY aux bridge
>> and the DP contorller need to be described in DT.
>>
>> In addition, the cdn_dp_parse_next_bridge_dt() will parses it and
>> determines whether to register one or two bridges.
>>
>> Since there is only one DP controller, only one of the PHY ports can
>> output at a time. The key is how to switch between different PHYs,
>> which is handled by cdn_dp_switch_port() and cdn_dp_enable().
>>
>> There are two cases:
>>
>> 1. Neither bridge is enabled. In this case, both bridges can
>> independently read the EDID, and the PHY port may switch before
>> reading the EDID.
>>
>> 2. One bridge is already enabled. In this case, other bridges are not
>> allowed to read the EDID. So we will try to return the cached EDID.
>>
>> Since the scenario of two ports plug in at the same time is rare,
>> I don't have a board which support two TypeC connector to test this.
>> Therefore, I tested forced switching on a single PHY port, as well as
>> output using a fake PHY port alongside a real PHY port.
>>
>> Signed-off-by: Chaoyi Chen <chaoyi.chen@xxxxxxxxxxxxxx>
>> ---
>>
>> (no changes since v11)
>>
>> Changes in v10:
>> - Fix refcount usage of drm_bridge.
>> - Remove unused cdn_dp_next_bridge type.
>>
>> Changes in v9:
>> - Select DRM_AUX_HPD_BRIDGE when using DP driver.
>>
>> (no changes since v7)
>>
>> Changes in v6:
>> - Rename some variable names.
>> - Attach the DP bridge to the next bridge.
>>
>> Changes in v5:
>> - By parsing the HPD bridge chain, set the connector's of_node to the
>> of_node corresponding to the USB-C connector.
>> - Return EDID cache when other port is already enabled.
>> ---
>>
>>  drivers/gpu/drm/rockchip/Kconfig       |   1 +
>>  drivers/gpu/drm/rockchip/cdn-dp-core.c | 325 ++++++++++++++++++++-----
>>  drivers/gpu/drm/rockchip/cdn-dp-core.h |  18 +-
>>  3 files changed, 287 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
>> index b7b025814e72..10d9f29a3d44 100644
>> --- a/drivers/gpu/drm/rockchip/Kconfig
>> +++ b/drivers/gpu/drm/rockchip/Kconfig
>> @@ -56,6 +56,7 @@ config ROCKCHIP_CDN_DP
>>  	select DRM_DISPLAY_HELPER
>>  	select DRM_BRIDGE_CONNECTOR
>>  	select DRM_DISPLAY_DP_HELPER
>> +	select DRM_AUX_HPD_BRIDGE
>>  	help
>>  	  This selects support for Rockchip SoC specific extensions
>>  	  for the cdn DP driver. If you want to enable Dp on
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> index 1e27301584a4..0bc3d248c266 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> @@ -27,16 +27,17 @@
>>  #include "cdn-dp-core.h"
>>  #include "cdn-dp-reg.h"
>>
>> -static inline struct cdn_dp_device *bridge_to_dp(struct drm_bridge *bridge)
>> +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port *prev_port,
>> +			      struct cdn_dp_port *port);
>> +
>> +static inline struct cdn_dp_bridge *bridge_to_dp_bridge(struct drm_bridge *bridge)
>>  {
>> -	return container_of(bridge, struct cdn_dp_device, bridge);
>> +	return container_of(bridge, struct cdn_dp_bridge, bridge);
>>  }
>>
>> -static inline struct cdn_dp_device *encoder_to_dp(struct drm_encoder *encoder)
>> +static inline struct cdn_dp_device *bridge_to_dp(struct drm_bridge *bridge)
>>  {
>> -	struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>> -
>> -	return container_of(rkencoder, struct cdn_dp_device, encoder);
>> +	return bridge_to_dp_bridge(bridge)->parent;
>>  }
>>
>>  #define GRF_SOC_CON9		0x6224
>> @@ -191,14 +192,27 @@ static int cdn_dp_get_sink_count(struct cdn_dp_device *dp, u8 *sink_count)
>>  static struct cdn_dp_port *cdn_dp_connected_port(struct cdn_dp_device *dp)
>>  {
>>  	struct cdn_dp_port *port;
>> -	int i, lanes;
>> +	int i, lanes[MAX_PHY];
>>
>>  	for (i = 0; i < dp->ports; i++) {
>>  		port = dp->port[i];
>> -		lanes = cdn_dp_get_port_lanes(port);
>> -		if (lanes)
>> +		lanes[i] = cdn_dp_get_port_lanes(port);
>> +		if (!dp->next_bridge_valid)
>>  			return port;
>>  	}
>> +
>> +	if (dp->next_bridge_valid) {
>> +		/* If more than one port is available, pick the last active port */
>> +		if (dp->active_port > 0 && lanes[dp->active_port])
>> +			return dp->port[dp->active_port];
>> +
>> +		/* If the last active port is not available, pick an available port in order */
>> +		for (i = 0; i < dp->bridge_count; i++) {
>> +			if (lanes[i])
>> +				return dp->port[i];
>> +		}
>> +	}
>> +
>>  	return NULL;
>>  }
>>
>> @@ -253,12 +267,45 @@ static const struct drm_edid *
>>  cdn_dp_bridge_edid_read(struct drm_bridge *bridge, struct drm_connector *connector)
>>  {
>>  	struct cdn_dp_device *dp = bridge_to_dp(bridge);
>> -	const struct drm_edid *drm_edid;
>> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
>> +	struct cdn_dp_port *port = dp->port[dp_bridge->id];
>> +	struct cdn_dp_port *prev_port;
>> +	const struct drm_edid *drm_edid = NULL;
>> +	int i, ret;
>>
>>  	mutex_lock(&dp->lock);
>> +
>> +	/* More than one port is available */
>> +	if (dp->bridge_count > 1 && !port->phy_enabled) {
>> +		for (i = 0; i < dp->bridge_count; i++) {
>> +			/* Another port already enable */
>> +			if (dp->bridge_list[i] != dp_bridge && dp->bridge_list[i]->enabled)
>> +				goto get_cache;
>> +			/* Find already enabled port */
>> +			if (dp->port[i]->phy_enabled)
>> +				prev_port = dp->port[i];
>> +		}
>> +
>> +		/* Switch to current port */
>> +		if (prev_port) {
>> +			ret = cdn_dp_switch_port(dp, prev_port, port);
>> +			if (ret)
>> +				goto get_cache;
>> +		}
>> +	}
>> +
>>  	drm_edid = drm_edid_read_custom(connector, cdn_dp_get_edid_block, dp);
>> +	/* replace edid cache */
>> +	if (dp->edid_cache[dp_bridge->id])
>> +		drm_edid_free(dp->edid_cache[dp_bridge->id]);
>> +	dp->edid_cache[dp_bridge->id] = drm_edid_dup(drm_edid);
>> +
>>  	mutex_unlock(&dp->lock);
>> +	return drm_edid;
>>
>> +get_cache:
>> +	drm_edid = drm_edid_dup(dp->edid_cache[dp_bridge->id]);
>> +	mutex_unlock(&dp->lock);
>>  	return drm_edid;
>>  }
>>
>> @@ -267,12 +314,13 @@ cdn_dp_bridge_mode_valid(struct drm_bridge *bridge,
>>  			 const struct drm_display_info *display_info,
>>  			 const struct drm_display_mode *mode)
>>  {
>> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
>>  	struct cdn_dp_device *dp = bridge_to_dp(bridge);
>>  	u32 requested, actual, rate, sink_max, source_max = 0;
>>  	u8 lanes, bpc;
>>
>>  	/* If DP is disconnected, every mode is invalid */
>> -	if (!dp->connected)
>> +	if (!dp_bridge->connected || !dp->connected)
>>  		return MODE_BAD;
>>
>>  	switch (display_info->bpc) {
>> @@ -550,6 +598,54 @@ static bool cdn_dp_check_link_status(struct cdn_dp_device *dp)
>>  	return drm_dp_channel_eq_ok(link_status, min(port->lanes, sink_lanes));
>>  }
>>
>> +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port *prev_port,
>> +			      struct cdn_dp_port *port)
>> +{
>> +	int ret;
>> +
>> +	if (dp->active)
>> +		return 0;
>> +
>> +	ret = cdn_dp_disable_phy(dp, prev_port);
>> +	if (ret)
>> +		goto out;
>> +	ret = cdn_dp_enable_phy(dp, port);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = cdn_dp_get_sink_capability(dp);
>> +	if (ret) {
>> +		cdn_dp_disable_phy(dp, port);
>> +		goto out;
>> +	}
>> +
>> +	dp->active = true;
>> +	dp->lanes = port->lanes;
>> +
>> +	if (!cdn_dp_check_link_status(dp)) {
>> +		dev_info(dp->dev, "Connected with sink; re-train link\n");
>> +
>> +		ret = cdn_dp_train_link(dp);
>> +		if (ret) {
>> +			dev_err(dp->dev, "Training link failed: %d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
>> +		if (ret) {
>> +			dev_err(dp->dev, "Failed to idle video %d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		ret = cdn_dp_config_video(dp);
>> +		if (ret)
>> +			dev_err(dp->dev, "Failed to configure video: %d\n", ret);
>> +	}
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>>  static void cdn_dp_display_info_update(struct cdn_dp_device *dp,
>>  				       struct drm_display_info *display_info)
>>  {
>> @@ -571,6 +667,7 @@ static void cdn_dp_display_info_update(struct cdn_dp_device *dp,
>>  static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_atomic_state *state)
>>  {
>>  	struct cdn_dp_device *dp = bridge_to_dp(bridge);
>> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
>>  	struct drm_connector *connector;
>>  	int ret, val;
>>
>> @@ -580,7 +677,7 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
>>
>>  	cdn_dp_display_info_update(dp, &connector->display_info);
>>
>> -	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, &dp->encoder.encoder);
>> +	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, &dp_bridge->encoder.encoder);
>>  	if (ret < 0) {
>>  		DRM_DEV_ERROR(dp->dev, "Could not get vop id, %d", ret);
>>  		return;
>> @@ -599,6 +696,9 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
>>
>>  	mutex_lock(&dp->lock);
>>
>> +	if (dp->next_bridge_valid)
>> +		dp->active_port = dp_bridge->id;
>> +
>>  	ret = cdn_dp_enable(dp);
>>  	if (ret) {
>>  		DRM_DEV_ERROR(dp->dev, "Failed to enable bridge %d\n",
>> @@ -631,6 +731,7 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
>>  		goto out;
>>  	}
>>
>> +	dp_bridge->enabled = true;
>>  out:
>>  	mutex_unlock(&dp->lock);
>>  }
>> @@ -638,9 +739,11 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
>>  static void cdn_dp_bridge_atomic_disable(struct drm_bridge *bridge, struct drm_atomic_state *state)
>>  {
>>  	struct cdn_dp_device *dp = bridge_to_dp(bridge);
>> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
>>  	int ret;
>>
>>  	mutex_lock(&dp->lock);
>> +	dp_bridge->enabled = false;
>>
>>  	if (dp->active) {
>>  		ret = cdn_dp_disable(dp);
>> @@ -827,6 +930,16 @@ static int cdn_dp_audio_mute_stream(struct drm_bridge *bridge,
>>  	return ret;
>>  }
>>
>> +static void cdn_dp_bridge_hpd_notify(struct drm_bridge *bridge,
>> +			   enum drm_connector_status status)
>> +{
>> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
>> +	struct cdn_dp_device *dp = bridge_to_dp(bridge);
>> +
>> +	dp->bridge_list[dp_bridge->id]->connected = status == connector_status_connected;
>> +	schedule_work(&dp->event_work);
>> +}
>> +
>>  static const struct drm_bridge_funcs cdn_dp_bridge_funcs = {
>>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>> @@ -837,6 +950,7 @@ static const struct drm_bridge_funcs cdn_dp_bridge_funcs = {
>>  	.atomic_disable = cdn_dp_bridge_atomic_disable,
>>  	.mode_valid = cdn_dp_bridge_mode_valid,
>>  	.mode_set = cdn_dp_bridge_mode_set,
>> +	.hpd_notify = cdn_dp_bridge_hpd_notify,
>>
>>  	.dp_audio_prepare = cdn_dp_audio_prepare,
>>  	.dp_audio_mute_stream = cdn_dp_audio_mute_stream,
>> @@ -885,7 +999,8 @@ static void cdn_dp_pd_event_work(struct work_struct *work)
>>  {
>>  	struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device,
>>  						event_work);
>> -	int ret;
>> +	bool connected;
>> +	int i, ret;
>>
>>  	mutex_lock(&dp->lock);
>>
>> @@ -944,9 +1059,12 @@ static void cdn_dp_pd_event_work(struct work_struct *work)
>>
>>  out:
>>  	mutex_unlock(&dp->lock);
>> -	drm_bridge_hpd_notify(&dp->bridge,
>> -			      dp->connected ? connector_status_connected
>> -					    : connector_status_disconnected);
>> +	for (i = 0; i < dp->bridge_count; i++) {
>> +		connected = dp->connected && dp->bridge_list[i]->connected;
>> +		drm_bridge_hpd_notify(&dp->bridge_list[i]->bridge,
>> +				      connected ? connector_status_connected
>> +						: connector_status_disconnected);
>> +	}
>>  }
>>
>>  static int cdn_dp_pd_event(struct notifier_block *nb,
>> @@ -966,28 +1084,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb,
>>  	return NOTIFY_DONE;
>>  }
>>
>> -static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>> +static int cdn_bridge_add(struct device *dev,
>> +			  struct drm_bridge *bridge,
>> +			  struct drm_bridge *next_bridge,
>> +			  struct drm_encoder *encoder)
>>  {
>>  	struct cdn_dp_device *dp = dev_get_drvdata(dev);
>> -	struct drm_encoder *encoder;
>> +	struct drm_device *drm_dev = dp->drm_dev;
>> +	struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;
>>  	struct drm_connector *connector;
>> -	struct cdn_dp_port *port;
>> -	struct drm_device *drm_dev = data;
>> -	int ret, i;
>> -
>> -	ret = cdn_dp_parse_dt(dp);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	dp->drm_dev = drm_dev;
>> -	dp->connected = false;
>> -	dp->active = false;
>> -	dp->active_port = -1;
>> -	dp->fw_loaded = false;
>> -
>> -	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
>> -
>> -	encoder = &dp->encoder.encoder;
>> +	int ret;
>>
>>  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev,
>>  							     dev->of_node);
>> @@ -1002,26 +1108,35 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>>
>>  	drm_encoder_helper_add(encoder, &cdn_dp_encoder_helper_funcs);
>>
>> -	dp->bridge.ops =
>> -			DRM_BRIDGE_OP_DETECT |
>> -			DRM_BRIDGE_OP_EDID |
>> -			DRM_BRIDGE_OP_HPD |
>> -			DRM_BRIDGE_OP_DP_AUDIO;
>> -	dp->bridge.of_node = dp->dev->of_node;
>> -	dp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>> -	dp->bridge.hdmi_audio_dev = dp->dev;
>> -	dp->bridge.hdmi_audio_max_i2s_playback_channels = 8;
>> -	dp->bridge.hdmi_audio_spdif_playback = 1;
>> -	dp->bridge.hdmi_audio_dai_port = -1;
>> -
>> -	ret = devm_drm_bridge_add(dev, &dp->bridge);
>> +	bridge->ops =
>> +		DRM_BRIDGE_OP_DETECT |
>> +		DRM_BRIDGE_OP_EDID |
>> +		DRM_BRIDGE_OP_HPD |
>> +		DRM_BRIDGE_OP_DP_AUDIO;
>> +	bridge->of_node = dp->dev->of_node;
>> +	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
>> +	bridge->hdmi_audio_dev = dp->dev;
>> +	bridge->hdmi_audio_max_i2s_playback_channels = 8;
>> +	bridge->hdmi_audio_spdif_playback = 1;
>> +	bridge->hdmi_audio_dai_port = -1;
>> +
>> +	ret = devm_drm_bridge_add(dev, bridge);
>>  	if (ret)
>>  		return ret;
>>
>> -	ret = drm_bridge_attach(encoder, &dp->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> +	ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>  	if (ret)
>>  		return ret;
>>
>> +	if (next_bridge) {
>> +		ret = drm_bridge_attach(encoder, next_bridge, bridge,
>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> +		if (ret)
>> +			return ret;
>> +
>> +		last_bridge = drm_bridge_chain_get_last_bridge(bridge->encoder);
>> +	}
>> +
>>  	connector = drm_bridge_connector_init(drm_dev, encoder);
>>  	if (IS_ERR(connector)) {
>>  		ret = PTR_ERR(connector);
>> @@ -1029,8 +1144,100 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>>  		return ret;
>>  	}
>>
>> +	if (last_bridge)
>> +		connector->fwnode = fwnode_handle_get(of_fwnode_handle(last_bridge->of_node));
>> +
>>  	drm_connector_attach_encoder(connector, encoder);
>>
>> +	return 0;
>> +}
>> +
>> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
>> +{
>> +	struct device_node *np = dp->dev->of_node;
>> +	struct device_node *port __free(device_node) = of_graph_get_port_by_id(np, 1);
>> +	struct drm_bridge *bridge;
>> +	int count = 0;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	/* If device use extcon, do not use hpd bridge */
>> +	for (i = 0; i < dp->ports; i++) {
>> +		if (dp->port[i]->extcon) {
>> +			dp->bridge_count = 1;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	/* One endpoint may correspond to one next bridge. */
>> +	for_each_of_graph_port_endpoint(port, dp_ep) {
>> +		struct device_node *next_bridge_node __free(device_node) =
>> +			of_graph_get_remote_port_parent(dp_ep);
>> +
>> +		bridge = of_drm_find_bridge(next_bridge_node);
>> +		if (!bridge) {
>> +			ret = -EPROBE_DEFER;
>> +			goto out;
>> +		}
>> +
>> +		drm_bridge_get(bridge);
>> +		dp->next_bridge_valid = true;
>> +		dp->next_bridge_list[count] = bridge;
> 
> Correct, but the drm_bridge_get() slightly far away from the assignement is
> a bit misleading. I hadn't seen it initially so I suspected a missing get.
> 
> I suggest to do it in a single statement, for clarity:
> 
> 		dp->next_bridge_list[count] = bridgedrm_bridge_get(bridge);
>

I think the bridgedrm_bridge_get here seems to be a typo?

Anyway, thank you very much for your efforts on the bridge lifetime.
I will fix this in the next version :)

> With that changed, at least for the bridge lifetime aspects, you can add to
> the next version my:
> 
>  Reviewed-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
Best, 
Chaoyi