[PATCH] clk: check for disabled clock-provider in of_clk_get_hw_from_clkspec

[PATCH] clk: check for disabled clock-provider in of_clk_get_hw_from_clkspec

[Heiko Stuebner]

of_clk_get_hw_from_clkspec checks all available clock-providers by
compairing their of-nodes to the one from the clkspec. If no matching
clock-provider is found, the function returns EPROBE_DEFER to cause a
re-check at a later date.

If a matching clock-provider is found, a authoritative answer can be
retrieved from it whether the clock exists or not.

This does not take into account that the clock-provider may never appear,
because it's node is disabled. This can happen for example when a clock
is optional, provided by a separate block which just never gets enabled.

One example of this happening is the rk3588's VOP, which has optional
additional display-clock-supplies coming from PLLs inside the hdmiphy
blocks. These can be used for better rates, but the system will also
work without them.

The problem around that is described in the followups to:
https://lore.kernel.org/dri-devel/20250215-vop2-hdmi1-disp-modes-v1-3-81962a7151d6@collabora.com/

As we already know the of-node of the presumed clock-provider, just add
a check via of_device_is_available whether this is a "valid" device node.
This prevents ethernal defer-loops.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk.c | 4 ++++
 1 file changed, 4 insertions(+)

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) {
-- 
2.47.2

Re: [PATCH] clk: check for disabled clock-provider in of_clk_get_hw_from_clkspec

[Sebastian Reichel]

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

Hi,

On Thu, Feb 20, 2025 at 11:54:48PM +0100, Heiko Stuebner wrote:
> of_clk_get_hw_from_clkspec checks all available clock-providers by
> compairing their of-nodes to the one from the clkspec. If no matching
> clock-provider is found, the function returns EPROBE_DEFER to cause a
> re-check at a later date.
> 
> If a matching clock-provider is found, a authoritative answer can be
> retrieved from it whether the clock exists or not.
> 
> This does not take into account that the clock-provider may never appear,
> because it's node is disabled. This can happen for example when a clock
> is optional, provided by a separate block which just never gets enabled.
> 
> One example of this happening is the rk3588's VOP, which has optional
> additional display-clock-supplies coming from PLLs inside the hdmiphy
> blocks. These can be used for better rates, but the system will also
> work without them.
> 
> The problem around that is described in the followups to:
> https://lore.kernel.org/dri-devel/20250215-vop2-hdmi1-disp-modes-v1-3-81962a7151d6@collabora.com/
> 
> As we already know the of-node of the presumed clock-provider, just add
> a check via of_device_is_available whether this is a "valid" device node.
> This prevents ethernal defer-loops.

if there is a respin:

s/ethernal/eternal

> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---

Also seems useful for having a quick final error if a referenced
clock node has not been enabled by accident during board bringup.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Thanks,

-- Sebastian

>  drivers/clk/clk.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> 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) {
> -- 
> 2.47.2
> 

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

Re: [PATCH] clk: check for disabled clock-provider in of_clk_get_hw_from_clkspec

[Cristian Ciocaltea]

Hi,

On 2/21/25 12:54 AM, Heiko Stuebner wrote:
> of_clk_get_hw_from_clkspec checks all available clock-providers by
> compairing their of-nodes to the one from the clkspec. If no matching
> clock-provider is found, the function returns EPROBE_DEFER to cause a
> re-check at a later date.
> 
> If a matching clock-provider is found, a authoritative answer can be

Nit: s/a authoritative/an authoritative/

> retrieved from it whether the clock exists or not.
> 
> This does not take into account that the clock-provider may never appear,
> because it's node is disabled. This can happen for example when a clock
> is optional, provided by a separate block which just never gets enabled.
> 
> One example of this happening is the rk3588's VOP, which has optional
> additional display-clock-supplies coming from PLLs inside the hdmiphy
> blocks. These can be used for better rates, but the system will also
> work without them.
> 
> The problem around that is described in the followups to:
> https://lore.kernel.org/dri-devel/20250215-vop2-hdmi1-disp-modes-v1-3-81962a7151d6@collabora.com/
> 
> As we already know the of-node of the presumed clock-provider, just add
> a check via of_device_is_available whether this is a "valid" device node.
> This prevents ethernal defer-loops.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/clk.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> 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) {

Without this patch applied, when disabling hdmi0 and hdptxphy0 nodes on
Rock5B, rockchip-drm module fails to probe and hdmi1 output cannot be
used anymore:

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

After applying the patch, I confirm the error is gone and hdmi1 is
functional again.  Hence,

Tested-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Thanks,
Cristian