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@xxxxxxxxxxxxx>
>>
>> Add device tree support for Synopsys DesignWare HDMI RX
>> Controller.
>>
>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>> Tested-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>> Co-developed-by: Dingxian Wen <shawn.wen@xxxxxxxxxxxxxx>
>> Signed-off-by: Dingxian Wen <shawn.wen@xxxxxxxxxxxxxx>
>> Signed-off-by: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>> ---
>> .../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
