Hi, At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@xxxxxxxxxxxxx> wrote: >On 6/3/24 4:08 PM, neil.armstrong@xxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxx >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
