| Message ID | 20240613-hdmi-tx-v4-4-4af17e468699@freebox.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HDMI TX support in msm8998 | expand |
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
On Fri, Jun 14, 2024 at 01:55:46AM GMT, Konrad Dybcio wrote: > > > 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.. Yes. The whole qfprom / hdcp part needs to be reworked, but it should not stop the platform from flowing in. > > > + > > + 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 It might be, or it might be not. DSI interfaces also use MDP_CLK on those platforms. > > > + <&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 Let me quote the discussion on #linux-msm <lumag> jhugo, do you know anything about MNOC_AHB_CLK / MISC_AHB_CLK? Should they be enabled for HDMI to work? <jhugo> lumag: MNOC AHB, yes <jhugo> lumag: MISC, probably > > + clock-names = > > + "mdp_core", > > + "iface", > > + "core", > > + "alt_iface", > > + "extp", > > + "bus", > > + "mnoc", > > + "iface_mmss"; > > + [...] > > + > > + 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 I'd prefer to keep it. I think the original DT used one of LN_BB clocks here, so it might be that the HDMI uses CXO2 / LN_BB instead of the main CXO. If somebody can check, which clock is actually used for the HDMI, it would be really great. > > Konrad > > -- > linux-phy mailing list > linux-phy@lists.infradead.org > https://lists.infradead.org/mailman/listinfo/linux-phy
On 14.06.2024 12:33 PM, Dmitry Baryshkov wrote: > On Fri, Jun 14, 2024 at 01:55:46AM GMT, Konrad Dybcio wrote: >> >> [...] >> 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 > > I'd prefer to keep it. I think the original DT used one of LN_BB clocks > here, so it might be that the HDMI uses CXO2 / LN_BB instead of the main > CXO. > > If somebody can check, which clock is actually used for the HDMI, it > would be really great. +CC jhugo - could you please take a look? Konrad
On 6/15/2024 5:35 AM, Konrad Dybcio wrote: > On 14.06.2024 12:33 PM, Dmitry Baryshkov wrote: >> On Fri, Jun 14, 2024 at 01:55:46AM GMT, Konrad Dybcio wrote: >>> >>> > > [...] > >>> 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 >> >> I'd prefer to keep it. I think the original DT used one of LN_BB clocks >> here, so it might be that the HDMI uses CXO2 / LN_BB instead of the main >> CXO. >> >> If somebody can check, which clock is actually used for the HDMI, it >> would be really great. > > +CC jhugo - could you please take a look? > > Konrad Documentation is not great but it looks like CXO from what little I can find. -Jeff
On 17/06/2024 19:14, Jeffrey Hugo wrote: > On 6/15/2024 5:35 AM, Konrad Dybcio wrote: >> On 14.06.2024 12:33 PM, Dmitry Baryshkov wrote: >>> On Fri, Jun 14, 2024 at 01:55:46AM GMT, Konrad Dybcio wrote: >>>> >>>> >> >> [...] >> >>>> 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 >>> >>> I'd prefer to keep it. I think the original DT used one of LN_BB clocks >>> here, so it might be that the HDMI uses CXO2 / LN_BB instead of the main >>> CXO. >>> >>> If somebody can check, which clock is actually used for the HDMI, it >>> would be really great. >> >> +CC jhugo - could you please take a look? >> >> Konrad > > Documentation is not great but it looks like CXO from what little I can > find. If I read correctly, the conclusion of this sub-thread is that the clock tree described in the patch is correct? HDMI controller: + clocks = <&mmcc MDSS_MDP_CLK>, + <&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>, + <&mmcc MISC_AHB_CLK>; + clock-names = + "mdp_core", + "iface", + "core", + "alt_iface", + "extp", + "bus", + "mnoc", + "iface_mmss"; PHY: + clocks = <&mmcc MDSS_AHB_CLK>, + <&gcc GCC_HDMI_CLKREF_CLK>, + <&rpmcc RPM_SMD_XO_CLK_SRC>; + clock-names = "iface", + "ref", + "xo"; Regards
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi index ba5e873f0f35f..5c53957da61c5 100644 --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi @@ -2785,7 +2785,7 @@ mmcc: clock-controller@c8c0000 { <&mdss_dsi0_phy 0>, <&mdss_dsi1_phy 1>, <&mdss_dsi1_phy 0>, - <0>, + <&hdmi_phy 0>, <0>, <0>, <&gcc GCC_MMSS_GPLL0_DIV_CLK>; @@ -2890,6 +2890,14 @@ dpu_intf2_out: endpoint { remote-endpoint = <&mdss_dsi1_in>; }; }; + + port@2 { + reg = <2>; + + dpu_intf3_out: endpoint { + remote-endpoint = <&hdmi_in>; + }; + }; }; }; @@ -3045,6 +3053,96 @@ mdss_dsi1_phy: phy@c996400 { status = "disabled"; }; + + hdmi: hdmi-tx@c9a0000 { + compatible = "qcom,hdmi-tx-8998"; + reg = <0x0c9a0000 0x50c>, + <0x00780000 0x6220>, + <0x0c9e0000 0x2c>; + reg-names = "core_physical", + "qfprom_physical", + "hdcp_physical"; + + interrupt-parent = <&mdss>; + interrupts = <8>; + + clocks = <&mmcc MDSS_MDP_CLK>, + <&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>, + <&mmcc MISC_AHB_CLK>; + 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>; + + 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"; + + status = "disabled"; + }; }; venus: video-codec@cc00000 {