[PATCH 0/4] Improve Rockchip VOP2 display modes handling on RK3588 HDMI1

[PATCH 0/4] Improve Rockchip VOP2 display modes handling on RK3588 HDMI1

[Cristian Ciocaltea]

As a followup to getting basic HDMI1 output support [1] merged upstream,
make use of the HDMI1 PHY PLL to provide better VOP2 display modes
handling for the second HDMI output port on RK3588 SoC, similarly to
what has been achieved recently for HDMI0 [2].

Additionally, enable HDMI1 output on Rockchip RK3588 EVB1.

[1] https://lore.kernel.org/lkml/20241211-rk3588-hdmi1-v2-0-02cdca22ff68@collabora.com/
[2] https://lore.kernel.org/lkml/20250204-vop2-hdmi0-disp-modes-v3-0-d71c6a196e58@collabora.com/

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Cristian Ciocaltea (4):
      drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI1
      arm64: dts: rockchip: Enable HDMI1 PHY clk provider on RK3588
      arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588
      arm64: dts: rockchip: Enable HDMI1 on rk3588-evb1

 arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts | 42 ++++++++++++++++++++++--
 arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi   | 22 +++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c     | 26 ++++++++++++++-
 3 files changed, 87 insertions(+), 3 deletions(-)
---
base-commit: 0ae0fa3bf0b44c8611d114a9f69985bf451010c3
change-id: 20250215-vop2-hdmi1-disp-modes-ea8da428bc8e

[PATCH 1/4] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI1

[Cristian Ciocaltea]

The RK3588 specific implementation is currently quite limited in terms
of handling the full range of display modes supported by the connected
screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
few of them.

Additionally, it doesn't cope well with non-integer refresh rates like
59.94, 29.97, 23.98, etc.

Make use of HDMI1 PHY PLL as a more accurate DCLK source to handle
all display modes up to 4K@60Hz.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index afc946ead87091373605e59dbca281a9e91bea57..f1700b2fabf0b0d000cf3c9cf5f06ca791b87499 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -216,6 +216,7 @@ struct vop2 {
 	struct clk *aclk;
 	struct clk *pclk;
 	struct clk *pll_hdmiphy0;
+	struct clk *pll_hdmiphy1;
 
 	/* optional internal rgb encoder */
 	struct rockchip_rgb *rgb;
@@ -2270,11 +2271,14 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 	 * Switch to HDMI PHY PLL as DCLK source for display modes up
 	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
 	 */
-	if (vop2->pll_hdmiphy0 && clock <= VOP2_MAX_DCLK_RATE) {
+	if ((vop2->pll_hdmiphy0 || vop2->pll_hdmiphy1) && clock <= VOP2_MAX_DCLK_RATE) {
 		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
 			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
 
 			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
+				if (!vop2->pll_hdmiphy0)
+					break;
+
 				if (!vp->dclk_src)
 					vp->dclk_src = clk_get_parent(vp->dclk);
 
@@ -2284,6 +2288,20 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
 				break;
 			}
+
+			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI1) {
+				if (!vop2->pll_hdmiphy1)
+					break;
+
+				if (!vp->dclk_src)
+					vp->dclk_src = clk_get_parent(vp->dclk);
+
+				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy1);
+				if (ret < 0)
+					drm_warn(vop2->drm,
+						 "Could not switch to HDMI1 PHY PLL: %d\n", ret);
+				break;
+			}
 		}
 	}
 
@@ -3733,6 +3751,12 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 		return PTR_ERR(vop2->pll_hdmiphy0);
 	}
 
+	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
+	if (IS_ERR(vop2->pll_hdmiphy1)) {
+		drm_err(vop2->drm, "failed to get pll_hdmiphy1\n");
+		return PTR_ERR(vop2->pll_hdmiphy1);
+	}
+
 	vop2->irq = platform_get_irq(pdev, 0);
 	if (vop2->irq < 0) {
 		drm_err(vop2->drm, "cannot find irq for vop2\n");

-- 
2.48.1

[PATCH 2/4] arm64: dts: rockchip: Enable HDMI1 PHY clk provider on RK3588

[Cristian Ciocaltea]

Since commit c4b09c562086 ("phy: phy-rockchip-samsung-hdptx: Add clock
provider support"), the HDMI PHY PLL can be used as an alternative and
more accurate pixel clock source for VOP2 to improve display modes
handling on RK3588 SoC.

Add the missing #clock-cells property to allow using the clock provider
functionality of HDMI1 PHY.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
index 9bc5287bb6469138c2d9e2fcfec7984c830c2ce5..97e55990e0524ed447d182cef416190822bf67be 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
@@ -479,6 +479,7 @@ hdptxphy1: phy@fed70000 {
 		reg = <0x0 0xfed70000 0x0 0x2000>;
 		clocks = <&cru CLK_USB2PHY_HDPTXRXPHY_REF>, <&cru PCLK_HDPTX1>;
 		clock-names = "ref", "apb";
+		#clock-cells = <0>;
 		#phy-cells = <0>;
 		resets = <&cru SRST_HDPTX1>, <&cru SRST_P_HDPTX1>,
 			 <&cru SRST_HDPTX1_INIT>, <&cru SRST_HDPTX1_CMN>,

-- 
2.48.1

[PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Cristian Ciocaltea]

VOP2 on RK3588 is able to use the HDMI PHY PLL as an alternative and
more accurate pixel clock source to improve handling of display modes up
to 4K@60Hz on video ports 0, 1 and 2.

The HDMI1 PHY PLL clock source cannot be added directly to vop node in
rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an
optional feature and its PHY node belongs to a separate (extra) DT file.

Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its
clocks & clock-names properties in the extra DT file.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
index 97e55990e0524ed447d182cef416190822bf67be..1df8845bdc264b07601add3747b273f92091e7fa 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
@@ -542,3 +542,24 @@ pcie30phy: phy@fee80000 {
 		status = "disabled";
 	};
 };
+
+&vop {
+	clocks = <&cru ACLK_VOP>,
+		 <&cru HCLK_VOP>,
+		 <&cru DCLK_VOP0>,
+		 <&cru DCLK_VOP1>,
+		 <&cru DCLK_VOP2>,
+		 <&cru DCLK_VOP3>,
+		 <&cru PCLK_VOP_ROOT>,
+		 <&hdptxphy0>,
+		 <&hdptxphy1>;
+	clock-names = "aclk",
+		      "hclk",
+		      "dclk_vp0",
+		      "dclk_vp1",
+		      "dclk_vp2",
+		      "dclk_vp3",
+		      "pclk_vop",
+		      "pll_hdmiphy0",
+		      "pll_hdmiphy1";
+};

-- 
2.48.1

Re:[PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Jianfeng Liu]

Hi Cristian,

On Sat, 15 Feb 2025 02:55:39 +0200, Cristian Ciocaltea wrote:
>The HDMI1 PHY PLL clock source cannot be added directly to vop node in
>rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an
>optional feature and its PHY node belongs to a separate (extra) DT file.
>
>Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its
>clocks & clock-names properties in the extra DT file.

There are boards that only use hdmi1 such as ROCK 5 ITX. So there are two
choices for this board:

1, Enable hdptxphy0 as dependency of vop although it is not really used.

2, Overwrite vop node at board dts to make it only use hdptxphy1 like:

&vop {
	clocks = <&cru ACLK_VOP>,
		 <&cru HCLK_VOP>,
		 <&cru DCLK_VOP0>,
		 <&cru DCLK_VOP1>,
		 <&cru DCLK_VOP2>,
		 <&cru DCLK_VOP3>,
		 <&cru PCLK_VOP_ROOT>,
		 <&hdptxphy1>;
	clock-names = "aclk",
		      "hclk",
		      "dclk_vp0",
		      "dclk_vp1",
		      "dclk_vp2",
		      "dclk_vp3",
		      "pclk_vop",
		      "pll_hdmiphy1";
};

What do you think of these two method?

Best regards,
Jianfeng

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Heiko Stübner]

Am Montag, 17. Februar 2025, 03:44:37 MEZ schrieb Jianfeng Liu:
> Hi Cristian,
> 
> On Sat, 15 Feb 2025 02:55:39 +0200, Cristian Ciocaltea wrote:
> >The HDMI1 PHY PLL clock source cannot be added directly to vop node in
> >rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an
> >optional feature and its PHY node belongs to a separate (extra) DT file.
> >
> >Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its
> >clocks & clock-names properties in the extra DT file.
> 
> There are boards that only use hdmi1 such as ROCK 5 ITX. So there are two
> choices for this board:
> 
> 1, Enable hdptxphy0 as dependency of vop although it is not really used.
> 
> 2, Overwrite vop node at board dts to make it only use hdptxphy1 like:
> 
> &vop {
> 	clocks = <&cru ACLK_VOP>,
> 		 <&cru HCLK_VOP>,
> 		 <&cru DCLK_VOP0>,
> 		 <&cru DCLK_VOP1>,
> 		 <&cru DCLK_VOP2>,
> 		 <&cru DCLK_VOP3>,
> 		 <&cru PCLK_VOP_ROOT>,
> 		 <&hdptxphy1>;
> 	clock-names = "aclk",
> 		      "hclk",
> 		      "dclk_vp0",
> 		      "dclk_vp1",
> 		      "dclk_vp2",
> 		      "dclk_vp3",
> 		      "pclk_vop",
> 		      "pll_hdmiphy1";
> };
> 
> What do you think of these two method?

Going by the code in patch1 (+drm-misc) we have:
  vop2->pll_hdmiphy0 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy0");
+
  vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");

So the clock-reference to hdptxphy0 should just result in vop2->pll_hdmiphy0
being NULL and thus ignored further on?

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Cristian Ciocaltea]

On 2/17/25 4:33 PM, Heiko Stübner wrote:
> Am Montag, 17. Februar 2025, 03:44:37 MEZ schrieb Jianfeng Liu:
>> Hi Cristian,
>>
>> On Sat, 15 Feb 2025 02:55:39 +0200, Cristian Ciocaltea wrote:
>>> The HDMI1 PHY PLL clock source cannot be added directly to vop node in
>>> rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an
>>> optional feature and its PHY node belongs to a separate (extra) DT file.
>>>
>>> Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its
>>> clocks & clock-names properties in the extra DT file.
>>
>> There are boards that only use hdmi1 such as ROCK 5 ITX. So there are two
>> choices for this board:
>>
>> 1, Enable hdptxphy0 as dependency of vop although it is not really used.
>>
>> 2, Overwrite vop node at board dts to make it only use hdptxphy1 like:
>>
>> &vop {
>> 	clocks = <&cru ACLK_VOP>,
>> 		 <&cru HCLK_VOP>,
>> 		 <&cru DCLK_VOP0>,
>> 		 <&cru DCLK_VOP1>,
>> 		 <&cru DCLK_VOP2>,
>> 		 <&cru DCLK_VOP3>,
>> 		 <&cru PCLK_VOP_ROOT>,
>> 		 <&hdptxphy1>;
>> 	clock-names = "aclk",
>> 		      "hclk",
>> 		      "dclk_vp0",
>> 		      "dclk_vp1",
>> 		      "dclk_vp2",
>> 		      "dclk_vp3",
>> 		      "pclk_vop",
>> 		      "pll_hdmiphy1";
>> };
>>
>> What do you think of these two method?
> 
> Going by the code in patch1 (+drm-misc) we have:
>   vop2->pll_hdmiphy0 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy0");
> +
>   vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
> 
> So the clock-reference to hdptxphy0 should just result in vop2->pll_hdmiphy0
> being NULL and thus ignored further on?

Yep, that is the intended behavior.

@Jianfeng: Did you encounter any particular issue with the current approach?

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Jianfeng Liu]

Hi,

On Tue, 18 Feb 2025 01:33:34 +0200, Cristian Ciocaltea wrote:
>@Jianfeng: Did you encounter any particular issue with the current approach?

This patch is adding a dependency of hdptxphy1 to vop for all rk3588
boards, but not all rk3588 boards have dual hdmi, armsom sige7 is an
example. At runtime I will get errors because there is no hdptxphy1
enabled in devicetree:

[    2.945675] rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1

Overwrting vop node at board level to remove the dependency of hdptxphy1
can fix the issue.

Best regards,
Jianfeng

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Jianfeng Liu]

Hi Cristian,

No matter one or two hdmi ports the rk3588 boards have, most of
devicetrees in mainline kernel only have hdmi0 supported. After applying
this patch their hdmi0 support is broken.

So I recommend moving the vop clk part to board level devicetree.
Then support of hdmi0 won't be broken, and board maintainers can add
HDMI1 PHY PLL clk when they are adding hdmi1 support. I can add support
for orangepi 5 max and armsom w3 for reference by other developers.

Best regards,
Jianfeng

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Heiko Stübner]

Am Dienstag, 18. Februar 2025, 10:52:16 MEZ schrieb Jianfeng Liu:
> Hi Cristian,
> 
> No matter one or two hdmi ports the rk3588 boards have, most of
> devicetrees in mainline kernel only have hdmi0 supported. After applying
> this patch their hdmi0 support is broken.
> 
> So I recommend moving the vop clk part to board level devicetree.
> Then support of hdmi0 won't be broken, and board maintainers can add
> HDMI1 PHY PLL clk when they are adding hdmi1 support. I can add support
> for orangepi 5 max and armsom w3 for reference by other developers.

better, fix the VOP2 driver - both for the existing hdmi0 + this hdmi1
please.

I.e. the clock is optional, and the error you are seeing comes from the

       vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
       if (IS_ERR(vop2->pll_hdmiphy1)) {
               drm_err(vop2->drm, "failed to get pll_hdmiphy1\n");
               return PTR_ERR(vop2->pll_hdmiphy1);
       }

part. clk_get_optional is supposed to return NULL when clock-retrieval
causes a ENOENT error. Seemingly going to a clock controller in a disabled
node returns a different error?

So I guess step1, check what error is actually returned.
Step2 check if clk_get_optional need to be adapted or alternatively
catch the error in the vop2 and set the clock to NULL ourself in that case.


hdptxphy0 + hdpxphy1 _are_ valid supplies for the vop, so their reference
should be in the soc-dtsi and the kernel code should just figure things out
correctly. Wiggling with clocks in each board will cause headaches down
the road.


Heiko

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Jianfeng Liu]

Hi Heiko,

On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
>So I guess step1, check what error is actually returned.

I have checked that the return value is -517:

rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517

>Step2 check if clk_get_optional need to be adapted or alternatively
>catch the error in the vop2 and set the clock to NULL ourself in that case.

I tried the following patch to set the clock to NULL when clk_get_optional
failed with value -517, and hdmi0 is working now. There are also some
boards like rock 5 itx which only use hdmi1, I think we should also add
this logic to vop2->pll_hdmiphy0.

@@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 		return PTR_ERR(vop2->pll_hdmiphy0);
 	}
 
+	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
+	if (IS_ERR(vop2->pll_hdmiphy1)) {
+		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
+		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
+			vop2->pll_hdmiphy1 = NULL;
+		else
+			return PTR_ERR(vop2->pll_hdmiphy1);
+	}
+
 	vop2->irq = platform_get_irq(pdev, 0);
 	if (vop2->irq < 0) {
 		drm_err(vop2->drm, "cannot find irq for vop2\n");

Best regards,
Jianfeng

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

Hi,

On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:
> On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
> >So I guess step1, check what error is actually returned.
> 
> I have checked that the return value is -517:
> 
> rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517
> 
> >Step2 check if clk_get_optional need to be adapted or alternatively
> >catch the error in the vop2 and set the clock to NULL ourself in that case.
> 
> I tried the following patch to set the clock to NULL when clk_get_optional
> failed with value -517, and hdmi0 is working now. There are also some
> boards like rock 5 itx which only use hdmi1, I think we should also add
> this logic to vop2->pll_hdmiphy0.
> 
> @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
>  		return PTR_ERR(vop2->pll_hdmiphy0);
>  	}
>  
> +	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
> +	if (IS_ERR(vop2->pll_hdmiphy1)) {
> +		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
> +		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
> +			vop2->pll_hdmiphy1 = NULL;
> +		else
> +			return PTR_ERR(vop2->pll_hdmiphy1);
> +	}
> +

This first of all shows, that we should replace drm_err in this
place with dev_err_probe(), which hides -EPROBE_DEFER errors by
default and instead captures the reason for /sys/kernel/debug/devices_deferred.

Second what you are doing in the above suggestion will break kernel
configurations where VOP is built-in and the HDMI PHY is build as a
module.

But I also think it would be better to have the clocks defined in the
SoC level DT. I suppose that means vop2_bind would have to check if
the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID>
based on that. Considering there is the OF graph pointing from VOP
to the enabled HDMI controllers, it should be able to do that.

Greetings,

-- Sebastian

>  	vop2->irq = platform_get_irq(pdev, 0);
>  	if (vop2->irq < 0) {
>  		drm_err(vop2->drm, "cannot find irq for vop2\n");
> 
> Best regards,
> Jianfeng

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Heiko Stübner]

Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel:
> Hi,
> 
> On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:
> > On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
> > >So I guess step1, check what error is actually returned.
> > 
> > I have checked that the return value is -517:
> > 
> > rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517
> > 
> > >Step2 check if clk_get_optional need to be adapted or alternatively
> > >catch the error in the vop2 and set the clock to NULL ourself in that case.
> > 
> > I tried the following patch to set the clock to NULL when clk_get_optional
> > failed with value -517, and hdmi0 is working now. There are also some
> > boards like rock 5 itx which only use hdmi1, I think we should also add
> > this logic to vop2->pll_hdmiphy0.
> > 
> > @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
> >  		return PTR_ERR(vop2->pll_hdmiphy0);
> >  	}
> >  
> > +	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
> > +	if (IS_ERR(vop2->pll_hdmiphy1)) {
> > +		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
> > +		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
> > +			vop2->pll_hdmiphy1 = NULL;
> > +		else
> > +			return PTR_ERR(vop2->pll_hdmiphy1);
> > +	}
> > +
> 
> This first of all shows, that we should replace drm_err in this
> place with dev_err_probe(), which hides -EPROBE_DEFER errors by
> default and instead captures the reason for /sys/kernel/debug/devices_deferred.
> 
> Second what you are doing in the above suggestion will break kernel
> configurations where VOP is built-in and the HDMI PHY is build as a
> module.
> 
> But I also think it would be better to have the clocks defined in the
> SoC level DT. I suppose that means vop2_bind would have to check if
> the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID>
> based on that. Considering there is the OF graph pointing from VOP
> to the enabled HDMI controllers, it should be able to do that.


I was more thinking about fixing the correct thing, with something like:

----------- 8< ----------
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cf7720b9172f..50faafbf5dda 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
        if (!clkspec)
                return ERR_PTR(-EINVAL);

+       /* Check if node in clkspec is in disabled/fail state */
+       if (!of_device_is_available(clkspec->np))
+               return ERR_PTR(-ENOENT);
+
        mutex_lock(&of_clk_mutex);
        list_for_each_entry(provider, &of_clk_providers, link) {
                if (provider->node == clkspec->np) {
----------- 8< ----------

Because right now the clk framework does not handle nodes in
failed/disabled state and would defer indefinitly.

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]

Hi,

On Tue, Feb 18, 2025 at 03:53:06PM +0100, Heiko Stübner wrote:
> Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel:
> > On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:
> > > On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
> > > >So I guess step1, check what error is actually returned.
> > > 
> > > I have checked that the return value is -517:
> > > 
> > > rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517
> > > 
> > > >Step2 check if clk_get_optional need to be adapted or alternatively
> > > >catch the error in the vop2 and set the clock to NULL ourself in that case.
> > > 
> > > I tried the following patch to set the clock to NULL when clk_get_optional
> > > failed with value -517, and hdmi0 is working now. There are also some
> > > boards like rock 5 itx which only use hdmi1, I think we should also add
> > > this logic to vop2->pll_hdmiphy0.
> > > 
> > > @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
> > >  		return PTR_ERR(vop2->pll_hdmiphy0);
> > >  	}
> > >  
> > > +	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
> > > +	if (IS_ERR(vop2->pll_hdmiphy1)) {
> > > +		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
> > > +		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
> > > +			vop2->pll_hdmiphy1 = NULL;
> > > +		else
> > > +			return PTR_ERR(vop2->pll_hdmiphy1);
> > > +	}
> > > +
> > 
> > This first of all shows, that we should replace drm_err in this
> > place with dev_err_probe(), which hides -EPROBE_DEFER errors by
> > default and instead captures the reason for /sys/kernel/debug/devices_deferred.
> > 
> > Second what you are doing in the above suggestion will break kernel
> > configurations where VOP is built-in and the HDMI PHY is build as a
> > module.
> > 
> > But I also think it would be better to have the clocks defined in the
> > SoC level DT. I suppose that means vop2_bind would have to check if
> > the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID>
> > based on that. Considering there is the OF graph pointing from VOP
> > to the enabled HDMI controllers, it should be able to do that.
> 
> 
> I was more thinking about fixing the correct thing, with something like:
> 
> ----------- 8< ----------
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index cf7720b9172f..50faafbf5dda 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
>         if (!clkspec)
>                 return ERR_PTR(-EINVAL);
> 
> +       /* Check if node in clkspec is in disabled/fail state */
> +       if (!of_device_is_available(clkspec->np))
> +               return ERR_PTR(-ENOENT);
> +
>         mutex_lock(&of_clk_mutex);
>         list_for_each_entry(provider, &of_clk_providers, link) {
>                 if (provider->node == clkspec->np) {
> ----------- 8< ----------
> 
> Because right now the clk framework does not handle nodes in
> failed/disabled state and would defer indefinitly.

Also LGTM.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Cristian Ciocaltea]

Hi,

On 2/18/25 6:05 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Feb 18, 2025 at 03:53:06PM +0100, Heiko Stübner wrote:
>> Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel:
>>> On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:
>>>> On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
>>>>> So I guess step1, check what error is actually returned.
>>>>
>>>> I have checked that the return value is -517:
>>>>
>>>> rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517
>>>>
>>>>> Step2 check if clk_get_optional need to be adapted or alternatively
>>>>> catch the error in the vop2 and set the clock to NULL ourself in that case.
>>>>
>>>> I tried the following patch to set the clock to NULL when clk_get_optional
>>>> failed with value -517, and hdmi0 is working now. There are also some
>>>> boards like rock 5 itx which only use hdmi1, I think we should also add
>>>> this logic to vop2->pll_hdmiphy0.
>>>>
>>>> @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
>>>>  		return PTR_ERR(vop2->pll_hdmiphy0);
>>>>  	}
>>>>  
>>>> +	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
>>>> +	if (IS_ERR(vop2->pll_hdmiphy1)) {
>>>> +		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
>>>> +		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
>>>> +			vop2->pll_hdmiphy1 = NULL;
>>>> +		else
>>>> +			return PTR_ERR(vop2->pll_hdmiphy1);
>>>> +	}
>>>> +
>>>
>>> This first of all shows, that we should replace drm_err in this
>>> place with dev_err_probe(), which hides -EPROBE_DEFER errors by
>>> default and instead captures the reason for /sys/kernel/debug/devices_deferred.
>>>
>>> Second what you are doing in the above suggestion will break kernel
>>> configurations where VOP is built-in and the HDMI PHY is build as a
>>> module.
>>>
>>> But I also think it would be better to have the clocks defined in the
>>> SoC level DT. I suppose that means vop2_bind would have to check if
>>> the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID>
>>> based on that. Considering there is the OF graph pointing from VOP
>>> to the enabled HDMI controllers, it should be able to do that.
>>
>>
>> I was more thinking about fixing the correct thing, with something like:
>>
>> ----------- 8< ----------
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index cf7720b9172f..50faafbf5dda 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
>>         if (!clkspec)
>>                 return ERR_PTR(-EINVAL);
>>
>> +       /* Check if node in clkspec is in disabled/fail state */
>> +       if (!of_device_is_available(clkspec->np))
>> +               return ERR_PTR(-ENOENT);
>> +
>>         mutex_lock(&of_clk_mutex);
>>         list_for_each_entry(provider, &of_clk_providers, link) {
>>                 if (provider->node == clkspec->np) {
>> ----------- 8< ----------
>>
>> Because right now the clk framework does not handle nodes in
>> failed/disabled state and would defer indefinitly.
> 
> Also LGTM.

Thank you all for the feedback and proposed solutions!

I'm currently on leave and without access to any testing hw, but I'll be
back in a couple of days.

Regards,
Cristian

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

[Johannes Erdfelt]

On Tue, Feb 18, 2025, Heiko Stübner <heiko@sntech.de> wrote:
> I was more thinking about fixing the correct thing, with something like:
> 
> ----------- 8< ----------
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index cf7720b9172f..50faafbf5dda 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
>         if (!clkspec)
>                 return ERR_PTR(-EINVAL);
> 
> +       /* Check if node in clkspec is in disabled/fail state */
> +       if (!of_device_is_available(clkspec->np))
> +               return ERR_PTR(-ENOENT);
> +
>         mutex_lock(&of_clk_mutex);
>         list_for_each_entry(provider, &of_clk_providers, link) {
>                 if (provider->node == clkspec->np) {
> ----------- 8< ----------
> 
> Because right now the clk framework does not handle nodes in
> failed/disabled state and would defer indefinitly.

I've been testing the recent patches Jimmy Hon has posted to add the
Orange Pi 5 Ultra DT and I ran into this bug. The Ultra uses HDMI1 for
the HDMI TX.

This patch successfully fixes the issue I was seeing.

JE

[PATCH 4/4] arm64: dts: rockchip: Enable HDMI1 on rk3588-evb1

[Cristian Ciocaltea]

Add the necessary DT changes to enable the second HDMI output port on
Rockchip RK3588 EVB1.

While at it, switch the position of &vop_mmu and @vop to maintain the
alphabetical order.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts | 42 ++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
index 3fd0665cde2ca15cd309919ff751b00e0f53a400..27a7895595ee9fa2f5d5f3096cbe334c1d3792cf 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
@@ -132,6 +132,17 @@ hdmi0_con_in: endpoint {
 		};
 	};
 
+	hdmi1-con {
+		compatible = "hdmi-connector";
+		type = "a";
+
+		port {
+			hdmi1_con_in: endpoint {
+				remote-endpoint = <&hdmi1_out_con>;
+			};
+		};
+	};
+
 	pcie20_avdd0v85: regulator-pcie20-avdd0v85 {
 		compatible = "regulator-fixed";
 		regulator-name = "pcie20_avdd0v85";
@@ -364,10 +375,30 @@ hdmi0_out_con: endpoint {
 	};
 };
 
+&hdmi1 {
+	status = "okay";
+};
+
+&hdmi1_in {
+	hdmi1_in_vp1: endpoint {
+		remote-endpoint = <&vp1_out_hdmi1>;
+	};
+};
+
+&hdmi1_out {
+	hdmi1_out_con: endpoint {
+		remote-endpoint = <&hdmi1_con_in>;
+	};
+};
+
 &hdptxphy0 {
 	status = "okay";
 };
 
+&hdptxphy1 {
+	status = "okay";
+};
+
 &i2c2 {
 	status = "okay";
 
@@ -1371,11 +1402,11 @@ &usb_host1_xhci {
 	status = "okay";
 };
 
-&vop_mmu {
+&vop {
 	status = "okay";
 };
 
-&vop {
+&vop_mmu {
 	status = "okay";
 };
 
@@ -1385,3 +1416,10 @@ vp0_out_hdmi0: endpoint@ROCKCHIP_VOP2_EP_HDMI0 {
 		remote-endpoint = <&hdmi0_in_vp0>;
 	};
 };
+
+&vp1 {
+	vp1_out_hdmi1: endpoint@ROCKCHIP_VOP2_EP_HDMI1 {
+		reg = <ROCKCHIP_VOP2_EP_HDMI1>;
+		remote-endpoint = <&hdmi1_in_vp1>;
+	};
+};

-- 
2.48.1