Re: [PATCH v5 4/4] drm/bridge: analogix_dp: Add support for optional data-lanes mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The logic-to-physical lane mapping is mathematically inverted when programming the hardware, breaking support for asymmetric lane swap configurations.
- [Medium] Inconsistent endpoint resolution when reading the data-lanes property.
--

commit b6abf6d746df1b04ca60b36cd37e77da204d915a
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Add support for optional data-lanes mapping
    
Parse the optional 'data-lanes' device tree property to support
custom physical lane mapping configuration. If no valid configuration
is found, fall back to the default lane map automatically.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index f23c2ae6e10d0..2dfe7de5d3101 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1234,6 +1234,59 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {

[ ... ]

> +static int analogix_dp_dt_parse_lanes_map(struct analogix_dp_device *dp)
> +{
> +	struct video_info *video_info = &dp->video_info;
> +	struct device_node *endpoint;
> +	u32 tmp[LANE_COUNT4];
> +	u32 map[LANE_COUNT4] = {0, 1, 2, 3};
> +	bool used[LANE_COUNT4] = {false};
> +	int num_lanes;
> +	int ret, i;
> +
> +	memcpy(video_info->lane_map, map, sizeof(map));
> +
> +	num_lanes = drm_of_get_data_lanes_count_ep(dp->dev->of_node, 1, 0, 1,
> +						   video_info->max_lane_count);
> +	if (num_lanes < 0)
> +		return -EINVAL;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1);

[Severity: Medium]
Will this result in reading from an inconsistent endpoint if port 1 has
multiple endpoints?

The call to drm_of_get_data_lanes_count_ep() above explicitly passes 0
for the endpoint reg, but this call uses -1. If there are multiple
endpoints, this could return the first one it finds rather than endpoint@0,
which might lead to reading the lane map from a different endpoint than
the one used for the count.

> +	if (!endpoint)
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, num_lanes);

[ ... ]

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 38fd8d5014d29..45c7652645a13 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -48,16 +48,15 @@ void analogix_dp_stop_video(struct analogix_dp_device *dp)
>  	writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>  }
>  
> -void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
> +void analogix_dp_lane_mapping(struct analogix_dp_device *dp)
>  {
> +	u32 *lane_map = dp->video_info.lane_map;
>  	u32 reg;
>  
> -	if (enable)
> -		reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
> -		      LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
> -	else
> -		reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
> -		      LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
> +	reg = lane_map[0] << LANE0_MAP_SHIFT;
> +	reg |= lane_map[1] << LANE1_MAP_SHIFT;
> +	reg |= lane_map[2] << LANE2_MAP_SHIFT;
> +	reg |= lane_map[3] << LANE3_MAP_SHIFT;

[Severity: High]
Does this logic mathematically invert the lane mapping for asymmetric
configurations?

The device tree data-lanes property specifies a mapping from logical to
physical lanes, so that lane_map[logical_lane] = physical_lane.
However, when programming the hardware, it expects the logical lane index
to be placed into the bit-field corresponding to the physical lane.

Here, the code shifts the physical lane number by the logical lane's
bit-shift, effectively programming the logical lane into the physical lane's
field. This happens to work by coincidence for symmetric mappings like
<3 2 1 0> or <0 1 2 3>, but it will corrupt asymmetric configurations
like <1 2 3 0>.

Should the logical lane index instead be shifted into the physical lane's
bit-field? For example, using reg |= i << (lane_map[i] * 2).

>  
>  	writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604085220.2862986-1-damon.ding@rock-chips.com?part=4