From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from sender4-op-o11.zoho.com (sender4-op-o11.zoho.com [136.143.188.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6EA7130566C for ; Thu, 20 Aug 2026 14:45:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=136.143.188.11 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787237138; cv=pass; b=PvdzJQ4riLnl3urX/3UAvCGjAu+CHeHg8iWPbGcveA3An2AmTdBMp7W4f67G+46f71SmO1ZIIvUv4qHUGloFTSF8zjod8kx+31YUUrrYDqTtdnHCtZyFAovtW9+wbMpHYU4LMFp2e5x0vKmhA8osdNr7lu2RlewNAHinGOIPSIE= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787237138; c=relaxed/simple; bh=gZbsgX5OwmT7ALgF1UrQZXysLZ8N2FGn1r8EaIx4gSE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PFpnX2EmUcvxibHYgjVObsYGBOa9N6+FNR6MP207zF9GpULMfiCL9gw8FwS4kbFi20HwxSVpg/eFKbe2S+E1qLfxPbA2cy5nx4GYobJ+ohf4FwEnX/9DrKH7yjjE5j5rq701QZohEueOSmbfVlhkeXyMf9ZUYV9uRlvKisepYMk= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (1024-bit key) header.d=collabora.com header.i=sebastian.reichel@collabora.com header.b=EIRMG86I; arc=pass smtp.client-ip=136.143.188.11 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=collabora.com header.i=sebastian.reichel@collabora.com header.b="EIRMG86I" ARC-Seal: i=1; a=rsa-sha256; t=1787237115; cv=none; d=zohomail.com; s=zohoarc; b=UiRyjSrp8ZVdJwCrPSLx8EfZ6Ht58qyKoWmb2hMv6+t9DO2i8Xzlg1I6KC5c9dTprLAsFamr1w6K8Sdca6a55VOqR6tlghslULaeemTaB/KK2AxKymesh+ze2EZTvZAi3F9lE2der9HFGa69zbCWiaDMA5A6c+b0PatXSY/0NRY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1787237115; h=Content-Type:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:Subject:Subject:To:To:Message-Id:Reply-To; bh=yPfE/sHUiu2o6V0dJ0UBJAphD1W/z5Eu+CoSCFRLdRA=; b=R+DuOvDii37wM/+ldOeleFwLijbFDb9RO2EoEoBybu505ef3lNdlVGv7/b3ay9ueA9bB2zk03hTSsWk0Y770OMph/wrsx4vz5j05RWohm7sEb6HQ13x3ODdJoXzp9SC26qT6FdFN0W073qdZZsFoz9aljNaP4EPO3U8tKnUa24k= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=collabora.com; spf=pass smtp.mailfrom=sebastian.reichel@collabora.com; dmarc=pass header.from= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1787237115; s=zohomail; d=collabora.com; i=sebastian.reichel@collabora.com; h=Date:Date:From:From:To:To:Cc:Cc:Subject:Subject:Message-ID:MIME-Version:Content-Type:In-Reply-To:Message-Id:Reply-To; bh=yPfE/sHUiu2o6V0dJ0UBJAphD1W/z5Eu+CoSCFRLdRA=; b=EIRMG86IcaSmpPzhjoFaGNTPdAIBbcud+Cx4bpRqlPedm2xs5AZ543fCsH7F9rHq XQW5lm/JRv/Dz2pzdykL0pC31EDJOP+VlonkHxc4MIcr8wzVkSZQj1IWaFiCC7fonsw UoClxGKxzkO6VKDGImUazn/SJhkSl3/oHF7RNIA0= Received: by mx.zohomail.com with SMTPS id 1787237112729448.9155883822127; Thu, 20 Aug 2026 07:45:12 -0700 (PDT) Received: by venus (Postfix, from userid 1000) id 5D56918082F; Thu, 20 Aug 2026 16:45:09 +0200 (CEST) Date: Thu, 20 Aug 2026 16:45:09 +0200 From: Sebastian Reichel To: Luca Ceresoli Cc: Damon Ding , sashiko-reviews@lists.linux.dev, conor+dt@kernel.org, devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org, robh@kernel.org Subject: Re: [PATCH v6 3/5] drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties Message-ID: References: <20260707040613.1992841-1-damon.ding@rock-chips.com> <20260707040613.1992841-4-damon.ding@rock-chips.com> <20260707041711.4F7351F000E9@smtp.kernel.org> <00d3f58e-73bb-41da-a2df-dfeeca644b6f@rock-chips.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="tqb3otqvwl2eytwz" Content-Disposition: inline In-Reply-To: X-Zoho-Virus-Status: 1 X-Zoho-AV-Stamp: zmail-av-0.2.10.1.5.2/287.221.66 X-ZohoMailClient: External --tqb3otqvwl2eytwz Content-Type: text/plain; protected-headers=v1; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH v6 3/5] drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties MIME-Version: 1.0 Hi, On Wed, Aug 19, 2026 at 02:37:59PM +0200, Luca Ceresoli wrote: > Hello, >=20 > 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 manda= tory samsung DP DT properties --- > >>>> commit b573dba50acc6087122ffa4a65680322f4d004f8 > >>>> Author: Damon Ding > >>>> > >>>> 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 p= revent > >>>> link training failures caused by zero values. > >>>> > >>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/d= rivers/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(struc= t 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 AB= I? > >>>> > >>>> Since samsung,link-rate is a deprecated property, a modern schema-co= mpliant > >>>> 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 err= or? > >>>> > >>> > >>> I'm afraid it's difficult to provide a universal sensible default val= ue > >>> 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. Moreove= r, > >>> the current link training logic relies on these two parameters; omitt= ing > >>> 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 manda= tory, > >> after having been optional since 2016 (based on the commit in the Fixe= s: > >> 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, may= be > >> only with some rare low resolution or whatever? > >> > >> If the answer is "yes, there is a remote possibility that one sich dev= ice, > >> with some maybe rare configuration, will work", then no, we cannot make > >> this property mandatory now. There can be devices out there working wi= thout > >> 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 =3D 0, max_rate =3D 0) > > > > analogix_dp_full_link_train(max_lanes, max_rate): > > // Read sink capabilities via DPCD and sanitize them > > link_rate =3D read_dpcd(DP_MAX_LINK_RATE); // >=3D 0x06 after = fixup > > lane_count =3D read_dpcd(DP_MAX_LANE_COUNT);// >=3D 1 after fix= up > > > > // Clamp by the limits from DT > > if (link_rate > max_rate) // 0x06 > 0, always true > > link_rate =3D max_rate; // link_rate =3D 0 > > if (lane_count > max_lanes) // 1 > 0, always true > > lane_count =3D max_lanes; // lane_count =3D 0 > > > > // Configure TX with the zeroed values > > set_link_bandwidth(link_rate =3D 0) > > // writel() is only executed for bwtype =3D=3D 0x06/0x0a, > > // so LINK_BW_SET is never written and stays at reset value; > > // phy_configure() is called with link_rate =3D 0. > > > > set_lane_count(lane_count =3D 0) > > // writel(0, ANALOGIX_DP_LANE_COUNT_SET) enables 0 lanes; > > // phy_configure() is called with lanes =3D 0. > > > > // Program sink for link training > > drm_dp_dpcd_write(DP_LINK_BW_SET, {link_rate =3D 0, lane_count = =3D 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 =3D 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 >=3D 0x0= 6, > > lane_count >=3D 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. >=20 > Thank you very much for the detailed analysis! To it is enough to > proceed. I'll review your v7. Can't the default for missing properties just be changed to 4 lanes and 0x14 rate and the DP link training would automatically train to less lanes / rates based on hardware capabilities (which would render the properties basically useless except for a small speedup during link training)? Greetings, -- Sebastian --tqb3otqvwl2eytwz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEE72YNB0Y/i3JqeVQT2O7X88g7+poFAmqHEuoACgkQ2O7X88g7 +ppo7xAAoAFq+SlYLTwYJFRpvU7ZU/TcvGu3G8vMyVjzCBk5AsjgJ/o5MkoN5yLT vkgGjfeC0UMup7x9jUZsX2zNHVrVDjvkWEOgXmNwDHGYrQm0/hHwwtpf+oCSOkKp wfJDq8lxl8goiTGaT9dTb9W5zh++1I96SRqIRhi97jWgKQVbdkCpPFgi0J2RLpP8 DC/FbESwtEqOwGDthzhMaJfl2J4EoV3G4UZXRyXi0FsPLEm06jLasWZbasG3SBFq eXGEV3vDVoVmKAuEC62f/E+NTp+gpDAyoKuThrSfWZfJcse6MDPte/tLCArchN7Q 16mqk1N9IFwkQ19IZ9Y42dy0SgO1nMeXBjuyeX9lbIBRy+iUp8dQvcpOI21Ituue enGdBKGHjeAyGSTsBTCNhVZljLqseHc6wwEb2scambpWSFTNK5h/6HWKZEymrAnR 4oV3tBkNR04CSq2vsc8YkcUiXZKoGosfJNg24/MkoBhqrODMUsTYVhuh+9ic/gKy UIgzMiVCPUwu+UPXPHjk1TLw+lmnvvhSHASOdi4zf9lTVjWQFj6GjI0Zj232vOIu UBBoqV6eLxqn3HWsxb52/1G9PjghxZQ/rG06+kVZaZpDsqISnnYwmZ3X5joxsj+E JUTHdRiF5GZYZfnEKkqatiaRv+/BEAXyZKGyZSl6mIoi2BQGXlk= =hxts -----END PGP SIGNATURE----- --tqb3otqvwl2eytwz--