Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel:

quoted

On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:

quoted

On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:

quoted

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

quoted

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.