[PATCH v2] drm/msm: add msm8998 hdmi phy/pll support

[Dmitry Baryshkov]

On Mon, 8 Jul 2024 at 14:07, Marc Gonzalez <mgonzalez at freebox.fr> wrote:
>
> On 05/07/2024 16:34, Dmitry Baryshkov wrote:
>
> > On Thu, Jul 04, 2024 at 06:45:36PM GMT, Marc Gonzalez wrote:
> >
> >> From: Arnaud Vrac <avrac at freebox.fr>
> >>
> >> Ported from the downstream driver.
> >
> > Please write some sensible commit message.
>
> Here's an attempt at expanding the commit message:
>
> """
> This code adds support for the HDMI PHY block in the MSM8998.
> It is a copy & paste of the vendor driver downstream:
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/drivers/clk/msm/mdss/mdss-hdmi-pll-8998.c
> """

Add support for the HDMI PHY as present on the Qualcomm MSM8998
platform. The code is mostly c&p of the vendor code from msm-4.4,
kernel.lnx.4.4.r38-rel.

>
>
> >>  drivers/gpu/drm/msm/Makefile                   |   1 +
> >>  drivers/gpu/drm/msm/hdmi/hdmi.c                |   1 +
> >>  drivers/gpu/drm/msm/hdmi/hdmi.h                |   8 +
> >>  drivers/gpu/drm/msm/hdmi/hdmi_phy.c            |   5 +
> >>  drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c       | 789 +++++++++++++++++++++++++
> >>  drivers/gpu/drm/msm/registers/display/hdmi.xml |  89 +++
> >>  6 files changed, 893 insertions(+)
> >
> > - Missing changelog
>
> - Rebase onto v6.10
> - Move drivers/gpu/drm/msm/hdmi/hdmi.xml.h to drivers/gpu/drm/msm/registers/display/hdmi.xml
> - Add copyright attribution
> - Remove all dead/debug/temporary code
>
> > - Missing a pointer to bindings. Ideally bindings should come together with the driver.
>
> "qcom,hdmi-phy-8998" is defined in "HDMI TX support in msm8998" series (Acked by Rob Herring & Vinod Koul)

This (and the link to lore) ideally should be a part of the cover
letter or the comment below '---' in the patch.

>
> > I'm not going to check the math, but it looks pretty close to what we
> > have for msm8996.
>
> What is the consequence of this?

That I won't check the math :-D

>
>
> >> +static const char * const hdmi_phy_8998_reg_names[] = {
> >> +    "vdda-pll",
> >> +    "vdda-phy",
> >
> > Unless you have a strong reason to, please use vcca and vddio here, so
> > that we don't have unnecessary conditionals in schema.
>
> The vendor code uses vddio & vcca for msm8996;
> vdda-pll & vdda-phy for msm8998.
>
> Which is vcca? Which is vddio?

vddio = vdda-phy (1.8V)
vcca = vdda-pll (lower voltage)

> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8996-mdss-pll.dtsi
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-mdss-pll.dtsi#L121-172

-- 
With best wishes
Dmitry