Re:Re: [PATCH 00/14] Add initial support for the Rockchip RK3588 HDMI TX Controller

From: "Andy Yan" <andyshrk@163.com>
To: "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com>
Cc: neil.armstrong@linaro.org, "Heiko Stuebner" <heiko@sntech.de>,
	 "Andrzej Hajda" <andrzej.hajda@intel.com>,
	 "Robert Foss" <rfoss@kernel.org>,
	 "Laurent Pinchart" <Laurent.pinchart@ideasonboard.com>,
	 "Jonas Karlman" <jonas@kwiboo.se>,
	 "Jernej Skrabec" <jernej.skrabec@gmail.com>,
	 "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	 "Maxime Ripard" <mripard@kernel.org>,
	 "Thomas Zimmermann" <tzimmermann@suse.de>,
	 "David Airlie" <airlied@gmail.com>,
	 "Daniel Vetter" <daniel@ffwll.ch>,
	 "Sandy Huang" <hjc@rock-chips.com>,
	"Rob Herring" <robh@kernel.org>,
	 "Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	 "Conor Dooley" <conor+dt@kernel.org>,
	 "Mark Yao" <markyao0591@gmail.com>,
	 "Andy Yan" <andy.yan@rock-chips.com>,
	 dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	 linux-arm-kernel@lists.infradead.org,
	 linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
	 kernel@collabora.com, "Alexandre ARNOUD" <aarnoud@me.com>,
	 "Luis de Arquer" <ldearquer@gmail.com>,
	 "Algea Cao" <algea.cao@rock-chips.com>
Subject: Re:Re: [PATCH 00/14] Add initial support for the Rockchip RK3588 HDMI TX Controller
Date: Wed, 5 Jun 2024 17:25:07 +0800 (CST)	[thread overview]
Message-ID: <4456bc5a.9b2d.18fe7b76790.Coremail.andyshrk@163.com> (raw)
In-Reply-To: <b8066150-c147-4eb6-9f7a-2bd0268c274e@collabora.com>

Hi,

At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
>On 6/3/24 4:08 PM, neil.armstrong@linaro.org wrote:
>> Hi,
>> 
>> On 03/06/2024 15:03, Heiko Stuebner wrote:
>>> Am Montag, 3. Juni 2024, 14:14:17 CEST schrieb Andy Yan:
>>>> Hi Neil:
>>>>
>>>> On 6/3/24 16:55, Neil Armstrong wrote:
>>>>> Hi Christian,
>>>>>
>>>>> On 01/06/2024 15:12, Cristian Ciocaltea wrote:
>>>>>> The RK3588 SoC family integrates a Quad-Pixel (QP) variant of the
>>>>>> Synopsys DesignWare HDMI TX controller used in the previous SoCs.
>>>>>>
>>>>>> It is HDMI 2.1 compliant and supports the following features, among
>>>>>> others:
>>>>>>
>>>>> .
>>>>>
>>>>> ..
>>>>>
>>>>>> * SCDC I2C DDC access
>>>>>> * TMDS Scrambler enabling 2160p@60Hz with RGB/YCbCr4:4:4
>>>>>> * YCbCr4:2:0 enabling 2160p@60Hz at lower HDMI link speeds
>>>>>> * Multi-stream audio
>>>>>> * Enhanced Audio Return Channel (EARC)
>>>>> -> Those features were already supported by the HDMI 2.0a compliant
>>>>> HW, just
>>>>> list the _new_ features for HDMI 2.1
>>>>>
>>>>> I did a quick review of your patchset and I don't understand why you
>>>>> need
>>>>> to add a separate dw-hdmi-qp.c since you only need simple variants
>>>>> of the I2C
>>>>> bus, infoframe and bridge setup.
>>>>>
>>>>> Can you elaborate further ? isn't this Quad-Pixel (QP) TX controller
>>>>> version
>>>>> detectable at runtime ?
>>>>>
>>>>> I would prefer to keep a single dw-hdmi driver if possible.
>>>>
>>>>
>>>>
>>>> The QP HDMI controller is a completely different variant with totally
>>>> different
>>>> registers layout, see PATCH 13/14.
>>>> I think make it a separate driver will be easier for development and
>>>> maintenance.
>>>
>>> I'm with Andy here. Trying to navigate a driver for two IP blocks really
>>> sounds taxing especially when both are so different.
>
>Thank you all for the valuable feedback!
>
>> I agree, I just wanted more details than "variant of the
>> Synopsys DesignWare HDMI TX controller", if the register mapping is 100%
>> different, and does not match at all with the old IP, then it's indeed time
>> to make a brand new driver, but instead of doing a mix up, it's time to
>> extract
>> the dw-hdmi code that could be common helpers into a dw-hdmi-common module
>> and use them.
>
>Sounds good, will handle this in v2.
>
>> As I see, no "driver" code can be shared, only DRM plumbings, so perhaps
>> those
>> plumbing code should go into the DRM core ?
>> 
>> In any case, please add more details on the cover letter, including the
>> detailed
>> HW differrence and the design you chose so support this new IP.
>
>Andy, could you please help with a summary of the HW changes?
>The information I could provide is rather limited, since I don't have
>access to any DW IP datasheets and I'm also not familiar enough with the
>old variant.
>
 Accurately, we should refer to it as an entirely new IP，it has nothing in common with
the current mainline dw-hdmi。 The only  commonality is that they both come from
Synopsys DesignWare：
（1）It has a 100% different register mapping
（2）It supports FRL and DSC 
（3）different configuration flow in many places。

So I have the same feeling with Heiko and Maxime：
The DW_HDMI_QP should have a  separate driver and with it's  own CONFIG  such as DRM_DW_HDMI_QP  in Kconfig.
and the rockchip part should also be split from dw_hdmi-rockchip.c.  
I am sorry we mixed them in dw_hdmi-rockchip.c when we develop the bsp driver，but we really regretted this decision
when  we repeatedly broke compatibility with dw-hdmi on other socs。 

>> Neil
>> 
>>>
>>> Synopsis also created a new dsi controller for the DSI2 standard, with
>>> a vastly different registers layout.
>>>
>>> I guess at some point there is time to say this really is a new IP ;-) .
>>>
>>>
>>> Though while on that thought, I don't fully understand why both a
>>> compiled
>>> under the dw_hdmi kconfig symbol. People going for a minimal kernel might
>>> want one or the other, but not both for their specific board.
>
>Indeed, it makes sense to have a dedicated Kconfig option. This is
>mostly a leftover from downstream implementation, will fix in v2.
>
>Thanks again,
>Cristian
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel