Re: [PATCH 00/11] Subject: [PATCH 00/11] Add DRM support for Amlogic S4

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

Hello,

On Fri, Jan 10, 2025 at 6:39 AM Ao Xu via B4 Relay
<devnull+ao.xu.amlogic.com@kernel.org> 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