Re: [PATCH v4 4/4] arm64: dts: qcom: add HDMI nodes for msm8998

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 6/13/24 17:15, Marc Gonzalez wrote:
> From: Arnaud Vrac <avrac@freebox.fr>
> 
> Port device nodes from vendor code.
> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---

[...]

> +
> +			hdmi: hdmi-tx@c9a0000 {
> +				compatible = "qcom,hdmi-tx-8998";
> +				reg =	<0x0c9a0000 0x50c>,
> +					<0x00780000 0x6220>,
> +					<0x0c9e0000 0x2c>;
> +				reg-names = "core_physical",
> +					    "qfprom_physical",
> +					    "hdcp_physical";

The way qfprom is accessed (bypassing nvmem APIs) will need to be reworked..
but since we already have it like that on 8996, I'm fine with batch-reworking
these some time in the future..

> +
> +				interrupt-parent = <&mdss>;
> +				interrupts = <8>;
> +
> +				clocks = <&mmcc MDSS_MDP_CLK>,

Not sure if the MDP core clock is necessary here. Pretty sure it only
powers the display-controller@.. peripheral

> +					 <&mmcc MDSS_AHB_CLK>,
> +					 <&mmcc MDSS_HDMI_CLK>,
> +					 <&mmcc MDSS_HDMI_DP_AHB_CLK>,
> +					 <&mmcc MDSS_EXTPCLK_CLK>,
> +					 <&mmcc MDSS_AXI_CLK>,
> +					 <&mmcc MNOC_AHB_CLK>,

This one is an interconnect clock, drop it

> +					 <&mmcc MISC_AHB_CLK>;

And please confirm whether this one is necessary

> +				clock-names =
> +					"mdp_core",
> +					"iface",
> +					"core",
> +					"alt_iface",
> +					"extp",
> +					"bus",
> +					"mnoc",
> +					"iface_mmss";
> +
> +				phys = <&hdmi_phy>;
> +				#sound-dai-cells = <1>;
> +
> +				pinctrl-names = "default", "sleep";
> +				pinctrl-0 = <&hdmi_hpd_default
> +					     &hdmi_ddc_default
> +					     &hdmi_cec_default>;
> +				pinctrl-1 = <&hdmi_hpd_sleep
> +					     &hdmi_ddc_default
> +					     &hdmi_cec_default>;

property
property-names

please

and use <&foo>, <&bar>; for phandle arrays instead of <&foo bar>
(this is a really old dt and we still haven't got around to cleaning
up old junk for style issues..)

> +
> +				status = "disabled";
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					port@0 {
> +						reg = <0>;
> +						hdmi_in: endpoint {
> +							remote-endpoint = <&dpu_intf3_out>;
> +						};
> +					};
> +
> +					port@1 {
> +						reg = <1>;
> +						hdmi_out: endpoint {
> +						};
> +					};
> +				};
> +			};
> +
> +			hdmi_phy: hdmi-phy@c9a0600 {
> +				compatible = "qcom,hdmi-phy-8998";
> +				reg = <0x0c9a0600 0x18b>,
> +				      <0x0c9a0a00 0x38>,
> +				      <0x0c9a0c00 0x38>,
> +				      <0x0c9a0e00 0x38>,
> +				      <0x0c9a1000 0x38>,
> +				      <0x0c9a1200 0x0e8>;
> +				reg-names = "hdmi_pll",
> +					    "hdmi_tx_l0",
> +					    "hdmi_tx_l1",
> +					    "hdmi_tx_l2",
> +					    "hdmi_tx_l3",
> +					    "hdmi_phy";
> +
> +				#clock-cells = <0>;
> +				#phy-cells = <0>;
> +
> +				clocks = <&mmcc MDSS_AHB_CLK>,
> +					 <&gcc GCC_HDMI_CLKREF_CLK>,
> +					 <&rpmcc RPM_SMD_XO_CLK_SRC>;
> +				clock-names = "iface",
> +					      "ref",
> +					      "xo";

GCC_HDMI_CLKREF_CLK is a child of xo, so you can drop the latter.
It would also be worth confirming whether it's really powering the
PHY and not the TX.. You can test that by trying to only power on the
phy (e.g. call the phy_power_on or whatever APIs) with and without the
clock

Konrad