Re: [PATCH v14 1/3] arm64: dts: rockchip: Add device tree support for HDMI RX Controller

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

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 :)

-- 
Best regards,
Dmitry