Re: [PATCH v6 3/5] drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hello,

On Wed Aug 19, 2026 at 10:19 AM CEST, Damon Ding wrote:
> Hi Luca,
>
> On 8/19/2026 3:32 PM, Luca Ceresoli wrote:
>> Hello Damon,
>>
>> On Wed Jul 15, 2026 at 10:32 AM CEST, Damon Ding wrote:
>>> On 7/7/2026 12:17 PM, sashiko-bot@kernel.org wrote:
>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>> - [Critical] Failing probe when deprecated DT properties are missing breaks DT ABI and prevents their deprecation.
>>>> --
>>>>
>>>> --- Patch [3]: [PATCH v6 3/5] drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties ---
>>>> commit b573dba50acc6087122ffa4a65680322f4d004f8
>>>> Author: Damon Ding <damon.ding@rock-chips.com>
>>>>
>>>> drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties
>>>>
>>>> This commit reverts a change that made the samsung,link-rate and
>>>> samsung,lane-count properties optional for Exynos DP. It adds error checking
>>>> to fail the probe early if these properties are missing, aiming to prevent
>>>> link training failures caused by zero values.
>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>>> index 8cf6b73bceac0..293d8adfe9856 100644
>>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> [ ... ]
>>>>> @@ -1258,10 +1259,14 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
>>>>>    		 * NOTE: those property parseing code is used for
>>>>>    		 * providing backward compatibility for samsung platform.
>>>>>    		 */
>>>>> -		of_property_read_u32(dp_node, "samsung,link-rate",
>>>>> -				     &video_info->max_link_rate);
>>>>> -		of_property_read_u32(dp_node, "samsung,lane-count",
>>>>> -				     &video_info->max_lane_count);
>>>>> +		if (of_property_read_u32(dp_node, "samsung,link-rate", &val))
>>>>> +			return dev_err_probe(dp->dev, -EINVAL,
>>>>> +					     "Failed to get samsung,link-rate\n");
>>>>
>>>> [Severity: Critical]
>>>> Does failing the probe when this property is missing break the DT ABI?
>>>>
>>>> Since samsung,link-rate is a deprecated property, a modern schema-compliant
>>>> Device Tree might intentionally omit it. If a system boots with such a DT,
>>>> this check will fail the probe with -EINVAL and disable the display.
>>>>
>>>> Could the driver instead fall back to hardware maximums or sensible defaults
>>>> when the deprecated property is absent, rather than returning an error?
>>>>
>>>
>>> I'm afraid it's difficult to provide a universal sensible default value
>>> here.
>>>
>>> The samsung,lane-count and samsung,link-rate values vary significantly
>>> across Exynos DP device trees, so a generic fallback to hardware
>>> maximums cannot guarantee stable link training.
>>>
>>> Even though these two properties are marked deprecated, all existing DTS
>>> files in our projects have already populated them completely. Moreover,
>>> the current link training logic relies on these two parameters; omitting
>>> either will directly cause link training failures at runtime.
>>>
>>> Therefore, retaining the mandatory property check in probe is a
>>> reasonable approach for now.
>>
>> I'm still not convinced we should turn an optional property into mandatory,
>> after having been optional since 2016 (based on the commit in the Fixes:
>> tag).
>>
>> The sure thing is we cannot break existing devices which upgrade to a new
>> kernel.
>>
>> Se the question is: if a device is missing one of these properties, or
>> both, what happens? The video_info->max_link_rate and/or
>> video_info->max_lane_count will be zero, correct? And if they are zero, is
>> there even a remote possibility that the device will work somehow, maybe
>> only with some rare low resolution or whatever?
>>
>> If the answer is "yes, there is a remote possibility that one sich device,
>> with some maybe rare configuration, will work", then no, we cannot make
>> this property mandatory now. There can be devices out there working without
>> these proberties, and they would be broken.
>>
>> If the answer is "there is no way at all a device can work without one or
>> both properties", with a good explanation based on the code flow and
>> hardware docs, then we can consider this change.
>>
>
> Sorry for the confusion, I just submitted the v7 series which crossed
> with your reply.
>
> To answer your question: there is no way at all a device can work
> without these properties. Here is the code flow when either
> max_link_rate or max_lane_count is 0 (helped by AI):
>
>    analogix_dp_commit()
>      -> analogix_dp_full_link_train(dp, max_lanes = 0, max_rate = 0)
>
>    analogix_dp_full_link_train(max_lanes, max_rate):
>        // Read sink capabilities via DPCD and sanitize them
>        link_rate   = read_dpcd(DP_MAX_LINK_RATE); // >= 0x06 after fixup
>        lane_count  = read_dpcd(DP_MAX_LANE_COUNT);// >= 1 after fixup
>
>        // Clamp by the limits from DT
>        if (link_rate > max_rate)                // 0x06 > 0, always true
>            link_rate = max_rate;                // link_rate = 0
>        if (lane_count > max_lanes)              // 1 > 0, always true
>            lane_count = max_lanes;              // lane_count = 0
>
>        // Configure TX with the zeroed values
>        set_link_bandwidth(link_rate = 0)
>            // writel() is only executed for bwtype == 0x06/0x0a,
>            // so LINK_BW_SET is never written and stays at reset value;
>            // phy_configure() is called with link_rate = 0.
>
>        set_lane_count(lane_count = 0)
>            // writel(0, ANALOGIX_DP_LANE_COUNT_SET) enables 0 lanes;
>            // phy_configure() is called with lanes = 0.
>
>        // Program sink for link training
>        drm_dp_dpcd_write(DP_LINK_BW_SET, {link_rate = 0, lane_count = 0})
>            // DP spec requires link rate in {0x06, 0x0a, 0x14} and
>            // lane count in {1, 2, 4}. Writing zeros is illegal, so the
>            // sink cannot enter the training state.
>
>        // Training loop
>        for (lane = 0; lane < lane_count /* 0 */; lane++)
>            // loop body never executes; training_lane[] stays
>            // uninitialized and no training register is ever programmed
>
> Since the sanitized sink values are always non-zero (link_rate >= 0x06,
> lane_count >= 1), the clamping with a zero maximum unconditionally
> forces the training parameters to zero. Clock recovery can never be
> achieved, so link training fails deterministically.

Thank you very much for the detailed analysis! To it is enough to
proceed. I'll review your v7.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com