On Wednesday, March 06, 2024 01:50 IST, Heiko Stübner [off-list ref] wrote:

Hi again :-)

Am Dienstag, 5. März 2024, 20:05:02 CET schrieb Shreeya Patel:

quoted

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

quoted

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.

thanks, that will be helpful for me and everybody reading the dts later on

quoted

quoted

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.

I'm somewhat partial to the actual name, I was more getting at similar
names to keep things together.

General sorting rules are that &foo phandles are sorted alphabetically
in board devicetrees.

So having

&hdmirx {
	status = "okay";
};

&hdmirx_cma {
	status = "okay";
};

in the board dt, makes them stay together automatically ;-)

So if it's hdmirx + hdmirx_cma or hdmi_receiver + hdmi_receiver_cma
doesn't matter that much, just that they share a common basename.


I really want to stay away from allowing special rules for things as much
as possible, because that becomes a neverending story, so it's
alphabetical sorting.

But nothing prevents us from naming phandles in an intelligent way ;-) .

Makes sense to me, I'll use hdmi_receiver + hdmi_receiver_cma combination
to keep it consistent.

Thanks,
Shreeya Patel

Thanks
Heiko