Hello, On Fri, Jan 10, 2025 at 6:39 AM Ao Xu via B4 Relay <devnull+ao.xu.amlogic.com@xxxxxxxxxx> wrote: > > This patch series adds DRM support for the Amlogic S4-series SoCs. > Compared to the Amlogic G12-series, the S4-series introduces the following changes: Thanks for your patches. With this mail I'll try to summarize my understanding of the situation with the drm/meson driver as I'm not sure how familiar you are with previous discussions. > 1 The S4-series splits the HIU into three separate components: `sys_ctrl`, `pwr_ctrl`, and `clk_ctrl`. > As a result, VENC and VCLK drivers are updated with S4-specific compatible strings to accommodate these changes. Jerome has already commented on patch 3/11 that this mixes in too many IP blocks into one driver. This is not a new situation, the existing code is doing exactly the same. Jerome has previously sent a series which started "an effort to remove the use HHI syscon" [0] from the drm/meson hdmi driver. In the same mail he further notes: "[the patchset] stops short of making any controversial DT changes. To decouple the HDMI phy driver and main DRM driver, allowing the PHY to get hold of its registers, I believe a DT ABI break is inevitable. Ideally the register region of the PHY within the HHI should provided, not the whole HHI. That's pain for another day ..." For now I'm assuming you're familiar with device-tree ABI. If not then please let us know so we can elaborate further on this. My own notes for meson_dw_hdmi.c are: - we should not program HHI_HDMI_CLK_CNTL directly but go through CCF (common clock framework) instead (we should already have the driver for this in place) - we should not program HHI_MEM_PD_REG0 directly but go through the genpd/pmdomain framework (we should already have the driver for this in place) - for the HDMI PHY registers: on Meson8/8b/8m2 (those were 32-bit SoCs in case you're not familiar with them, they predate GXBB/GXL/...) I wrote a PHY driver (which is already upstream: drivers/phy/amlogic/phy-meson8-hdmi-tx.c) as that made most sense to me Then there's meson_venc.c which has two writes to HHI_GCLK_MPEG2. I think those should go through CCF instead of directly accessing this register. Also there's the VDAC registers in meson_encoder_cvbs.c: I think Neil suggested at one point to make it a PHY driver. I even started working on this (if you're curious: see [0] and [1]). DT ABI backwards compatibility is also a concern here. And finally there's the video clock tree programming in meson_vclk.c. My understanding here is that video PLL settings are very sensitive and require fine-tuning according to the desired output clock. Since it's a bunch of clocks I'd say that direct programming of the clock registers should be avoided and we need to go through CCF as well. But to me those register values are all magic (as I am not aware of any documentation that's available to me which describes how to determine the correct PLL register values - otherside the standard en/m/n/frac/lock and reset bits). I'm not saying that all of my thoughts are correct and immediately need to be put into code. Think of them more as an explanation to Jerome's reaction. I think what we need next is a discussion on how to move forward with device-tree ABI for new SoCs. Neil, Jerome: I'd like to hear your feedback on this. > 2 The S4-series secures access to HDMITX DWC and TOP registers, > requiring modifications to the driver to handle this new access method. This info should go into patch 1/11 to explain why the g12a compatible string cannot be used as fallback. Best regards, Martin [0] https://github.com/xdarklight/linux/commit/36e698edc25dc698a0d57b729a7a4587fafc0987 [1] https://github.com/xdarklight/linux/commit/824b792fdbd2d3c0b71b21e1105eca0aaad8daa6
