| Message ID | 20250306072842.287142-2-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable HDMI RX controller on RK3588 | expand |
Hi Dmitry, Am Donnerstag, 6. März 2025, 08:28:40 MEZ schrieb Dmitry Osipenko: > From: Shreeya Patel <shreeya.patel@collabora.com> > > Add device tree support for Synopsys DesignWare HDMI RX > Controller. > > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Co-developed-by: Dingxian Wen <shawn.wen@rock-chips.com> > Signed-off-by: Dingxian Wen <shawn.wen@rock-chips.com> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > .../dts/rockchip/rk3588-base-pinctrl.dtsi | 14 +++++ > .../arm64/boot/dts/rockchip/rk3588-extra.dtsi | 57 +++++++++++++++++++ > 2 files changed, 71 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > index 7f874c77410c..2d4b9986a177 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > @@ -594,6 +594,20 @@ hdmim0_tx1_hpd: hdmim0-tx1-hpd { > /* hdmim0_tx1_hpd */ > <1 RK_PA6 5 &pcfg_pull_none>; > }; > + > + /omit-if-no-ref/ > + hdmim1_rx: hdmim1-rx { > + rockchip,pins = > + /* hdmim1_rx_cec */ > + <3 RK_PD1 5 &pcfg_pull_none>, > + /* hdmim1_rx_scl */ > + <3 RK_PD2 5 &pcfg_pull_none_smt>, > + /* hdmim1_rx_sda */ > + <3 RK_PD3 5 &pcfg_pull_none_smt>, > + /* hdmim1_rx_hpdin */ > + <3 RK_PD4 5 &pcfg_pull_none>; > + }; > + what's the reason for duplicating these pinctrl entries? The base-pinctrl already contains a separate set of pins (and also a variant for the m0 set of pins), so why not check and use the already existing ones: hdmim1_rx_cec: hdmim1-rx-cec { rockchip,pins = /* hdmim1_rx_cec */ <3 RK_PD1 5 &pcfg_pull_none>; }; hdmim1_rx_hpdin: hdmim1-rx-hpdin { rockchip,pins = /* hdmim1_rx_hpdin */ <3 RK_PD4 5 &pcfg_pull_none>; }; hdmim1_rx_scl: hdmim1-rx-scl { rockchip,pins = /* hdmim1_rx_scl */ <3 RK_PD2 5 &pcfg_pull_none>; }; hdmim1_rx_sda: hdmim1-rx-sda { rockchip,pins = /* hdmim1_rx_sda */ <3 RK_PD3 5 &pcfg_pull_none>; }; Having multiple sets (even with differeing settings) will cause confusion later on. > /omit-if-no-ref/ > hdmim1_rx_cec: hdmim1-rx-cec { > rockchip,pins = > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi > index 4a950907ea6f..b7d06f93c8ce 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi > @@ -135,6 +159,39 @@ i2s10_8ch: i2s@fde00000 { > status = "disabled"; > }; > > + hdmi_receiver: hdmi_receiver@fdee0000 { > + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx"; > + reg = <0x0 0xfdee0000 0x0 0x6000>; > + power-domains = <&power RK3588_PD_VO1>; > + rockchip,grf = <&sys_grf>; > + rockchip,vo1-grf = <&vo1_grf>; > + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>; > + interrupt-names = "cec", "hdmi", "dma"; > + clocks = <&cru ACLK_HDMIRX>, > + <&cru CLK_HDMIRX_AUD>, > + <&cru CLK_CR_PARA>, > + <&cru PCLK_HDMIRX>, > + <&cru CLK_HDMIRX_REF>, > + <&cru PCLK_S_HDMIRX>, > + <&cru HCLK_VO1>; > + clock-names = "aclk", > + "audio", > + "cr_para", > + "pclk", > + "ref", > + "hclk_s_hdmirx", > + "hclk_vo1"; > + resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>, > + <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>; > + reset-names = "axi", "apb", "ref", "biu"; > + memory-region = <&hdmi_receiver_cma>; > + pinctrl-0 = <&hdmim1_rx>; > + pinctrl-names = "default"; hmm, this might be better living in the board dts? Heiko
On 3/6/25 16:06, Heiko Stübner wrote: > Hi Dmitry, > > Am Donnerstag, 6. März 2025, 08:28:40 MEZ schrieb Dmitry Osipenko: >> From: Shreeya Patel <shreeya.patel@collabora.com> >> >> Add device tree support for Synopsys DesignWare HDMI RX >> Controller. >> >> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> Co-developed-by: Dingxian Wen <shawn.wen@rock-chips.com> >> Signed-off-by: Dingxian Wen <shawn.wen@rock-chips.com> >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> .../dts/rockchip/rk3588-base-pinctrl.dtsi | 14 +++++ >> .../arm64/boot/dts/rockchip/rk3588-extra.dtsi | 57 +++++++++++++++++++ >> 2 files changed, 71 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi >> index 7f874c77410c..2d4b9986a177 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi >> @@ -594,6 +594,20 @@ hdmim0_tx1_hpd: hdmim0-tx1-hpd { >> /* hdmim0_tx1_hpd */ >> <1 RK_PA6 5 &pcfg_pull_none>; >> }; >> + >> + /omit-if-no-ref/ >> + hdmim1_rx: hdmim1-rx { >> + rockchip,pins = >> + /* hdmim1_rx_cec */ >> + <3 RK_PD1 5 &pcfg_pull_none>, >> + /* hdmim1_rx_scl */ >> + <3 RK_PD2 5 &pcfg_pull_none_smt>, >> + /* hdmim1_rx_sda */ >> + <3 RK_PD3 5 &pcfg_pull_none_smt>, >> + /* hdmim1_rx_hpdin */ >> + <3 RK_PD4 5 &pcfg_pull_none>; >> + }; >> + > > what's the reason for duplicating these pinctrl entries? > > The base-pinctrl already contains a separate set of pins (and also a > variant for the m0 set of pins), so why not check and use the already > existing ones: > > hdmim1_rx_cec: hdmim1-rx-cec { > rockchip,pins = > /* hdmim1_rx_cec */ > <3 RK_PD1 5 &pcfg_pull_none>; > }; > > hdmim1_rx_hpdin: hdmim1-rx-hpdin { > rockchip,pins = > /* hdmim1_rx_hpdin */ > <3 RK_PD4 5 &pcfg_pull_none>; > }; > > hdmim1_rx_scl: hdmim1-rx-scl { > rockchip,pins = > /* hdmim1_rx_scl */ > <3 RK_PD2 5 &pcfg_pull_none>; > }; > > hdmim1_rx_sda: hdmim1-rx-sda { > rockchip,pins = > /* hdmim1_rx_sda */ > <3 RK_PD3 5 &pcfg_pull_none>; > }; > > > Having multiple sets (even with differeing settings) will cause confusion > later on. Will try this variant, thanks! >> /omit-if-no-ref/ >> hdmim1_rx_cec: hdmim1-rx-cec { >> rockchip,pins = >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi >> index 4a950907ea6f..b7d06f93c8ce 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi >> @@ -135,6 +159,39 @@ i2s10_8ch: i2s@fde00000 { >> status = "disabled"; >> }; >> >> + hdmi_receiver: hdmi_receiver@fdee0000 { >> + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx"; >> + reg = <0x0 0xfdee0000 0x0 0x6000>; >> + power-domains = <&power RK3588_PD_VO1>; >> + rockchip,grf = <&sys_grf>; >> + rockchip,vo1-grf = <&vo1_grf>; >> + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>, >> + <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH 0>, >> + <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>; >> + interrupt-names = "cec", "hdmi", "dma"; >> + clocks = <&cru ACLK_HDMIRX>, >> + <&cru CLK_HDMIRX_AUD>, >> + <&cru CLK_CR_PARA>, >> + <&cru PCLK_HDMIRX>, >> + <&cru CLK_HDMIRX_REF>, >> + <&cru PCLK_S_HDMIRX>, >> + <&cru HCLK_VO1>; >> + clock-names = "aclk", >> + "audio", >> + "cr_para", >> + "pclk", >> + "ref", >> + "hclk_s_hdmirx", >> + "hclk_vo1"; >> + resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>, >> + <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>; >> + reset-names = "axi", "apb", "ref", "biu"; >> + memory-region = <&hdmi_receiver_cma>; >> + pinctrl-0 = <&hdmim1_rx>; >> + pinctrl-names = "default"; > > hmm, this might be better living in the board dts? HDMI RX controller is a part of the SoC, it's not specific to a particular board. If you meaning move it into rock5b.dts, then we definitely shouldn't do it. It's like moving PCIe controller node into a board DT :)
Am Donnerstag, 6. März 2025, 16:53:48 MEZ schrieb Dmitry Osipenko: > On 3/6/25 16:06, Heiko Stübner wrote: > > Hi Dmitry, > > > > Am Donnerstag, 6. März 2025, 08:28:40 MEZ schrieb Dmitry Osipenko: > >> From: Shreeya Patel <shreeya.patel@collabora.com> > >> > >> Add device tree support for Synopsys DesignWare HDMI RX > >> Controller. > >> > >> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> Co-developed-by: Dingxian Wen <shawn.wen@rock-chips.com> > >> Signed-off-by: Dingxian Wen <shawn.wen@rock-chips.com> > >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> --- > >> + hdmi_receiver: hdmi_receiver@fdee0000 { > >> + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx"; > >> + reg = <0x0 0xfdee0000 0x0 0x6000>; > >> + power-domains = <&power RK3588_PD_VO1>; > >> + rockchip,grf = <&sys_grf>; > >> + rockchip,vo1-grf = <&vo1_grf>; > >> + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>, > >> + <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH 0>, > >> + <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>; > >> + interrupt-names = "cec", "hdmi", "dma"; > >> + clocks = <&cru ACLK_HDMIRX>, > >> + <&cru CLK_HDMIRX_AUD>, > >> + <&cru CLK_CR_PARA>, > >> + <&cru PCLK_HDMIRX>, > >> + <&cru CLK_HDMIRX_REF>, > >> + <&cru PCLK_S_HDMIRX>, > >> + <&cru HCLK_VO1>; > >> + clock-names = "aclk", > >> + "audio", > >> + "cr_para", > >> + "pclk", > >> + "ref", > >> + "hclk_s_hdmirx", > >> + "hclk_vo1"; > >> + resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>, > >> + <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>; > >> + reset-names = "axi", "apb", "ref", "biu"; > >> + memory-region = <&hdmi_receiver_cma>; > >> + pinctrl-0 = <&hdmim1_rx>; > >> + pinctrl-names = "default"; > > > > hmm, this might be better living in the board dts? > > HDMI RX controller is a part of the SoC, it's not specific to a > particular board. If you meaning move it into rock5b.dts, then we > definitely shouldn't do it. It's like moving PCIe controller node into a > board DT :) sorry, should've been clearer. I meant the pinctrl. There are two sets of pins hdmim0-rx and hdmim1-rx which one is used is board specific. Can you check if "most" boards use the M1 variant? Rationale being, that if we know that most boards use the same set of pins, it's likely ok to just have the minority boards override it. Heiko
On 3/6/25 19:16, Heiko Stübner wrote: >>>> + pinctrl-0 = <&hdmim1_rx>; >>>> + pinctrl-names = "default"; >>> hmm, this might be better living in the board dts? >> HDMI RX controller is a part of the SoC, it's not specific to a >> particular board. If you meaning move it into rock5b.dts, then we >> definitely shouldn't do it. It's like moving PCIe controller node into a >> board DT
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi index 7f874c77410c..2d4b9986a177 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi @@ -594,6 +594,20 @@ hdmim0_tx1_hpd: hdmim0-tx1-hpd { /* hdmim0_tx1_hpd */ <1 RK_PA6 5 &pcfg_pull_none>; }; + + /omit-if-no-ref/ + hdmim1_rx: hdmim1-rx { + rockchip,pins = + /* hdmim1_rx_cec */ + <3 RK_PD1 5 &pcfg_pull_none>, + /* hdmim1_rx_scl */ + <3 RK_PD2 5 &pcfg_pull_none_smt>, + /* hdmim1_rx_sda */ + <3 RK_PD3 5 &pcfg_pull_none_smt>, + /* hdmim1_rx_hpdin */ + <3 RK_PD4 5 &pcfg_pull_none>; + }; + /omit-if-no-ref/ hdmim1_rx_cec: hdmim1-rx-cec { rockchip,pins = diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi index 4a950907ea6f..b7d06f93c8ce 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi @@ -7,6 +7,30 @@ #include "rk3588-extra-pinctrl.dtsi" / { + reserved-memory { + #address-cells = <2>; + #size-cells = <2>; + ranges; + + /* + * The 4k HDMI capture controller works only with 32bit + * phys addresses and doesn't support IOMMU. HDMI RX CMA + * must be reserved below 4GB. + * The size of 160MB was determined as follows: + * (3840 * 2160 pixels) * (4 bytes/pixel) * (2 frames/buffer) / 10^6 = 66MB + * To ensure sufficient support for practical use-cases, + * we doubled the 66MB value. + */ + hdmi_receiver_cma: hdmi-receiver-cma { + compatible = "shared-dma-pool"; + alloc-ranges = <0x0 0x0 0x0 0xffffffff>; + size = <0x0 (160 * 0x100000)>; /* 160MiB */ + alignment = <0x0 0x40000>; /* 64K */ + no-map; + status = "disabled"; + }; + }; + usb_host1_xhci: usb@fc400000 { compatible = "rockchip,rk3588-dwc3", "snps,dwc3"; reg = <0x0 0xfc400000 0x0 0x400000>; @@ -135,6 +159,39 @@ i2s10_8ch: i2s@fde00000 { status = "disabled"; }; + hdmi_receiver: hdmi_receiver@fdee0000 { + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx"; + reg = <0x0 0xfdee0000 0x0 0x6000>; + power-domains = <&power RK3588_PD_VO1>; + rockchip,grf = <&sys_grf>; + rockchip,vo1-grf = <&vo1_grf>; + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>, + <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH 0>, + <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>; + interrupt-names = "cec", "hdmi", "dma"; + clocks = <&cru ACLK_HDMIRX>, + <&cru CLK_HDMIRX_AUD>, + <&cru CLK_CR_PARA>, + <&cru PCLK_HDMIRX>, + <&cru CLK_HDMIRX_REF>, + <&cru PCLK_S_HDMIRX>, + <&cru HCLK_VO1>; + clock-names = "aclk", + "audio", + "cr_para", + "pclk", + "ref", + "hclk_s_hdmirx", + "hclk_vo1"; + resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>, + <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>; + reset-names = "axi", "apb", "ref", "biu"; + memory-region = <&hdmi_receiver_cma>; + pinctrl-0 = <&hdmim1_rx>; + pinctrl-names = "default"; + status = "disabled"; + }; + pcie3x4: pcie@fe150000 { compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie"; #address-cells = <3>;