Re: [PATCH v2 4/6] arm64: dts: rockchip: Add device tree support for HDMI RX Controller

On Tuesday, March 05, 2024 19:41 IST, Heiko Stübner [off-list ref] wrote:

Hi,

Hi Heiko,

Am Dienstag, 5. März 2024, 13:36:46 CET schrieb Shreeya Patel:

quoted

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 <redacted>
Signed-off-by: Dingxian Wen <redacted>
Signed-off-by: Shreeya Patel <redacted>
---
Changes in v2 :-
  - Fix some of the checkpatch errors and warnings
  - Rename resets, vo1-grf and HPD
  - Move hdmirx_cma node to the rk3588.dtsi file

 .../boot/dts/rockchip/rk3588-pinctrl.dtsi     | 41 ++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3588.dtsi      | 55 +++++++++++++++++++
 2 files changed, 96 insertions(+)

quoted

diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
index 5519c1430cb7..8adb98b99701 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi

@@ -7,6 +7,24 @@
 #include "rk3588-pinctrl.dtsi"
 
 / {
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;

add blank line here

quoted

+		/*
+		 * The 4k HDMI capture controller works only with 32bit
+		 * phys addresses and doesn't support IOMMU. HDMI RX CMA
+		 * must be reserved below 4GB.
+		 */
+		hdmirx_cma: hdmirx_cma {

phandles use "_", but node-names "-"

quoted

+			compatible = "shared-dma-pool";
+			alloc-ranges = <0x0 0x0 0x0 0xffffffff>;
+			size = <0x0 (160 * 0x100000)>; /* 160MiB */

The comment above that node, could elaborate where the value of 160MB
originates from. I assume it is to hold n-times of 4K frames or whatever,
but it would be helpful for people to be able to read that.

right, we did the following calculation to come up with this value :-
3840 * 2160 * 4 (bytes/pix) * 2 (frames/buffer) / 1000 / 1000 = 66M
and then we do the 2x times of this value to be on the safer side
and support all practical use-cases.

I'll add some more details to the comment in v3.

quoted

+			no-map;
+			status = "disabled";
+		};
+	};
+
 	pcie30_phy_grf: syscon@fd5b8000 {
 		compatible = "rockchip,rk3588-pcie3-phy-grf", "syscon";
 		reg = <0x0 0xfd5b8000 0x0 0x10000>;

@@ -85,6 +103,38 @@ i2s10_8ch: i2s@fde00000 {
 		status = "disabled";
 	};
 
+	hdmi_receiver: hdmi-receiver@fdee0000 {

Maybe rename the label to "hdmirx:" ... that way in a board enabling the
cma region, both nodes would stay close to each other?

Umm we already have receiver in the name so I am not sure if adding rx will be
a good idea. I was trying to keep it consistent with the names used in other device tree files.
In case you still feel otherwise then do let me know, I'll make the change.

quoted

+		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 436 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";

the driver uses of_reserved_mem_device_init(), so doesn't this node need
a "memory-region = <&hdmirx_cma>; or similar?

yes, we should have the memory-region property here. My bad, I'll correct this in v3.

quoted

+		resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>,
+			 <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>;
+		reset-names = "axi", "apb", "ref", "biu";
+		pinctrl-0 = <&hdmim1_rx>;
+		pinctrl-names = "default";
+		status = "disabled";
+	};
+
 	pcie3x4: pcie@fe150000 {
 		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
 		#address-cells = <3>;

@@ -339,3 +389,8 @@ pcie30phy: phy@fee80000 {
 		status = "disabled";
 	};
 };
+
+&hdmirx_cma {
+	status = "okay";
+};

I'd assume a board that enables &hdmi_receiver would also enable hdmirx_cma
and not the soc dtsi for _all_ boards?

Actually this node should be in the rock-5b.dts file instead of here.
v1 had it correct but I made a mistake in v2 :(
Thanks for pointing this out, I'll fix this and send a v3 soon.


Thanks,
Shreeya Patel

Thanks
Heiko


_______________________________________________
Kernel mailing list -- kernel@mailman.collabora.com
To unsubscribe send an email to kernel-leave@mailman.collabora.com
This list is managed by https://mailman.collabora.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel