| Message ID | 8cc61db5-2920-4dd1-8132-5af434fb05b1@freebox.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] arm64: dts: qcom: msm8998: add HDMI GPIOs | expand |
On Mon, May 27, 2024 at 05:40:15PM +0200, Marc Gonzalez wrote: > MSM8998 GPIO pin controller reference design defines: > > - CEC: pin 31 > - DDC: pin 32,33 > - HPD: pin 34 > > Downstream vendor code for reference: > > 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-pinctrl.dtsi#L2324-2400 > > mdss_hdmi_{cec,ddc,hpd}_{active,suspend} > > Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> > --- > arch/arm64/boot/dts/qcom/msm8998.dtsi | 42 +++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) While I don't see anything wrong with this patch, maybe it's better to include it into the patchset that adds all HDMI nodes to the msm8998.dtsi.
On 28/05/2024 14:57, Konrad Dybcio wrote: > On 5/28/24 14:45, Marc Gonzalez wrote: > >> On 28/05/2024 14:31, Konrad Dybcio wrote: >> >>> [...] >>> >>>> + hdmi_cec_default: hdmi-cec-default-state { >>>> + pins = "gpio31"; >>>> + function = "hdmi_cec"; >>>> + drive-strength = <2>; >>>> + bias-pull-up; >>>> + }; >>>> + >>>> + hdmi_cec_sleep: hdmi-cec-sleep-state { >>>> + pins = "gpio31"; >>>> + function = "hdmi_cec"; >>>> + drive-strength = <2>; >>>> + bias-pull-up; >>>> + }; >>> >>> It's super strange that both states are identical. Usually, the pull-up >>> is disabled and the GPIO is unmuxed (i.e. function = "gpio"). If that's >>> not the case for whatever reason, you can drop the sleep variants and >>> simply reference the leftover one from both pinctrl-0 and pinctrl-1. >> >> Patch is a direct translation of the vendor code: >> 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-pinctrl.dtsi#L2324-2400 >> >> I suppose it wouldn't be the first time that vendor code >> is doing something odd. >> >> Though, I'm a bit confused why you would single out hdmi-cec >> when hdmi_ddc is the same? > > As in, me in the above message or the vendor devicetree? > > If the former, it's just an example > > If the latter, the muxing function differs so they must have their > own separate nodes I meant: You (rightly) point out that hdmi_cec_default & hdmi_cec_sleep nodes are identical in my patch. I stated that, in fact, hdmi_ddc_default & hdmi_ddc_sleep nodes are ALSO identical in my patch. And the reason they are identical in my patch is because they are identical in the vendor code downstream: mdss_hdmi_cec_active & mdss_hdmi_cec_suspend mdss_hdmi_ddc_active & mdss_hdmi_ddc_suspend If I understand correctly, you are saying I should delete hdmi_cec_sleep and hdmi_ddc_sleep nodes, and refer to the default nodes in the hdmi pinctrl-1 prop? Regards
On 28/05/2024 02:45, Dmitry Baryshkov wrote: > While I don't see anything wrong with this patch, maybe it's better to > include it into the patchset that adds all HDMI nodes to the msm8998.dtsi. Here is my current diff: Do I just need to split it up, and it's good to go? (Doubtful++) diff --git a/Documentation/devicetree/bindings/phy/qcom,hdmi-phy-qmp.yaml b/Documentation/devicetree/bindings/phy/qcom,hdmi-phy-qmp.yaml index 83fe4b39b56f4..78607ee3e2e84 100644 --- a/Documentation/devicetree/bindings/phy/qcom,hdmi-phy-qmp.yaml +++ b/Documentation/devicetree/bindings/phy/qcom,hdmi-phy-qmp.yaml @@ -14,6 +14,7 @@ properties: compatible: enum: - qcom,hdmi-phy-8996 + - qcom,hdmi-phy-8998 reg: maxItems: 6 diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi index e5f051f5a92de..182d80c2ab942 100644 --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi @@ -1434,6 +1434,34 @@ blsp2_spi6_default: blsp2-spi6-default-state { drive-strength = <6>; bias-disable; }; + + hdmi_cec_default: hdmi-cec-default-state { + pins = "gpio31"; + function = "hdmi_cec"; + drive-strength = <2>; + bias-pull-up; + }; + + hdmi_ddc_default: hdmi-ddc-default-state { + pins = "gpio32", "gpio33"; + function = "hdmi_ddc"; + drive-strength = <2>; + bias-pull-up; + }; + + hdmi_hpd_default: hdmi-hpd-default-state { + pins = "gpio34"; + function = "hdmi_hot"; + drive-strength = <16>; + bias-pull-down; + }; + + hdmi_hpd_sleep: hdmi-hpd-sleep-state { + pins = "gpio34"; + function = "hdmi_hot"; + drive-strength = <2>; + bias-pull-down; + }; }; remoteproc_mss: remoteproc@4080000 { @@ -2757,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>; @@ -2862,6 +2890,14 @@ dpu_intf2_out: endpoint { remote-endpoint = <&mdss_dsi1_in>; }; }; + + port@2 { + reg = <2>; + + dpu_intf3_out: endpoint { + remote-endpoint = <&hdmi_in>; + }; + }; }; }; @@ -3017,6 +3053,103 @@ 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 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&mmcc MDSS_MDP_CLK>, + <&mmcc MNOC_AHB_CLK>, + <&mmcc MDSS_AHB_CLK>, + <&mmcc MDSS_AXI_CLK>, + <&mmcc MISC_AHB_CLK>, + <&mmcc MDSS_HDMI_CLK>, + <&mmcc MDSS_HDMI_DP_AHB_CLK>, + <&mmcc MDSS_EXTPCLK_CLK>; + clock-names = + "mdp_core", + "mnoc", + "iface", + "bus", + "iface_mmss", + "core", + "alt_iface", + "extp"; + + phys = <&hdmi_phy>; + phy-names = "hdmi_phy"; + + 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>; + + power-domains = <&rpmpd MSM8998_VDDCX>; + + #sound-dai-cells = <1>; + + 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>, + <&xo>; + clock-names = + "iface", + "ref", + "xo"; + power-domains = <&rpmpd MSM8998_VDDMX>; + + status = "disabled"; + }; }; venus: video-codec@cc00000 {
On 30.05.2024 3:06 PM, Dmitry Baryshkov wrote: [...] >> + power-domains = <&rpmpd MSM8998_VDDCX>; > > Is it? I don't see this in msm-4.4 Yes, it is. msm-4.4 says VDD_DIG which is &pm8998_s1_level which is CX As for the PHY, it's a safe guess to assume it's backed by MX. Maybe Jeff could chime in with a confirmation. Konrad
On Fri, 31 May 2024 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 30.05.2024 3:06 PM, Dmitry Baryshkov wrote: > > [...] > > >> + power-domains = <&rpmpd MSM8998_VDDCX>; > > > > Is it? I don't see this in msm-4.4 > > Yes, it is. msm-4.4 says VDD_DIG which is &pm8998_s1_level which is CX Oh, my... > > As for the PHY, it's a safe guess to assume it's backed by MX. Maybe > Jeff could chime in with a confirmation. > > Konrad
On 5/31/2024 7:55 AM, Dmitry Baryshkov wrote: > On Fri, 31 May 2024 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On 30.05.2024 3:06 PM, Dmitry Baryshkov wrote: >> >> [...] >> >>>> + power-domains = <&rpmpd MSM8998_VDDCX>; >>> >>> Is it? I don't see this in msm-4.4 >> >> Yes, it is. msm-4.4 says VDD_DIG which is &pm8998_s1_level which is CX > > Oh, my... > >> >> As for the PHY, it's a safe guess to assume it's backed by MX. Maybe >> Jeff could chime in with a confirmation. Sounds right, but I'm not finding anything in the documentation. -Jeff
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi index edf379c28e1e1..ec4e967ed9b2a 100644 --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi @@ -1424,6 +1424,48 @@ blsp2_spi6_default: blsp2-spi6-default-state { drive-strength = <6>; bias-disable; }; + + hdmi_cec_default: hdmi-cec-default-state { + pins = "gpio31"; + function = "hdmi_cec"; + drive-strength = <2>; + bias-pull-up; + }; + + hdmi_cec_sleep: hdmi-cec-sleep-state { + pins = "gpio31"; + function = "hdmi_cec"; + drive-strength = <2>; + bias-pull-up; + }; + + hdmi_ddc_default: hdmi-ddc-default-state { + pins = "gpio32", "gpio33"; + function = "hdmi_ddc"; + drive-strength = <2>; + bias-pull-up; + }; + + hdmi_ddc_sleep: hdmi-ddc-sleep-state { + pins = "gpio32", "gpio33"; + function = "hdmi_ddc"; + drive-strength = <2>; + bias-pull-up; + }; + + hdmi_hpd_default: hdmi-hpd-default-state { + pins = "gpio34"; + function = "hdmi_hot"; + drive-strength = <16>; + bias-pull-down; + }; + + hdmi_hpd_sleep: hdmi-hpd-sleep-state { + pins = "gpio34"; + function = "hdmi_hot"; + drive-strength = <2>; + bias-pull-down; + }; }; remoteproc_mss: remoteproc@4080000 {
MSM8998 GPIO pin controller reference design defines: - CEC: pin 31 - DDC: pin 32,33 - HPD: pin 34 Downstream vendor code for reference: 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-pinctrl.dtsi#L2324-2400 mdss_hdmi_{cec,ddc,hpd}_{active,suspend} Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> --- arch/arm64/boot/dts/qcom/msm8998.dtsi | 42 +++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)