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

[Jerome Brunet <jbrunet@baylibre.com>]

On Sun 12 Jan 2025 at 23:44, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> 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.

I completely agree with your description of the problem, that and
Krzysztof's remark on patch 6. This is not new with this series indeed,
so it does not introduce new problems really but it compounds the
existing ones.

Addressing those issues, if we want to, will get more difficult as more
support is added for sure.

>
>> 2 The S4-series secures access to HDMITX DWC and TOP registers,
>>   requiring modifications to the driver to handle this new access method.

Regmap buses are made for those cases where the registers are the same,
but accessed differently. There is an example in the patchset referenced by
Martin, to handle GXL and G12 diff.

> 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

-- 
Jerome