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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <lnuceofdwm6lgibworaghcujp6rrncvn4e2xc2vzltimjw3rqu@jur7x5cxt5ue>
Date: Tue, 18 Feb 2025 15:13:07 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Jianfeng Liu <liujianfeng1994@...il.com>
Cc: heiko@...ech.de, airlied@...il.com, andy.yan@...k-chips.com, 
	conor+dt@...nel.org, cristian.ciocaltea@...labora.com, devicetree@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, hjc@...k-chips.com, kernel@...labora.com, krzk+dt@...nel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, robh@...nel.org, simona@...ll.ch, 
	tzimmermann@...e.de
Subject: Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source
 to VOP2 on RK3588

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)