Message-ID: <10619139.qUNvkh4Gvn@diego>
Date: Thu, 27 Feb 2025 15:36:39 +0100
From: Heiko Stübner <heiko@...ech.de>
To: mturquette@...libre.com, Stephen Boyd <sboyd@...nel.org>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
 liujianfeng1994@...il.com, sebastian.reichel@...labora.com,
 cristian.ciocaltea@...labora.com
Subject:
 Re: [PATCH v2] clk: check for disabled clock-provider in
 of_clk_get_hw_from_clkspec

Hi Stephen,

Am Mittwoch, 26. Februar 2025, 23:32:28 MEZ schrieb Stephen Boyd:
> Quoting Heiko Stuebner (2025-02-22 14:37:33)
> > 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, an 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 eternal defer-loops.
> > 
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> > Tested-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
> > Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> > ---
> 
> Applied to clk-next (unless this needs to fix something urgent?)

the area where this affects something for me is slated for 6.15, so
personally I see no urge to have this in 6.14 - especially as the effect
is present for so long already and nobody complained.

Though it looks like clk-next hasn't been pushed yet :-) .


> Please write a unit test (or many).

Had to look a bit ... never noticed the kunit dtso files before.
But will look into that :-) .

> I also wonder if we should use a
> different return value so that we don't try to look up the clk by name
> (see clk_core_fill_parent_index()). We could go even further and stop
> trying to find the clk over and over again too. Maybe -ENODEV can
> indicate that and we can cache that parent entry value so we stop
> trying.

Pffff ... no clue :-)

I.e. in the case I have, we're coming from clk_get_optional() [0].
which is supposed to just return NULL if the clock is not found, so at
least for the consumer view, the internals are not fixed and we could have
different "internal" error codes.

Not sure if more direct users of the of_clk_ functions would be affected
though?


In the case above, the optional clock is just a single one coming from a
phy-block, which may probe later (needing defer) or never (if disabled).

As for caching the ENODEV, I'm not sure how often we'd experience that?

Like "normally" you have that one big clock-controller + maybe a number
of smaller ones + maybe some blocks that expose one or two clocks
to one specific user - in my case the hdmi-phy exposing its hdmi-pll
for a nicer rate to the display controller when generating a hdmi output.

Does a case exist where some never-probed clock controller would have
so many clock-consumers that caching that single of-property check
would matter?


Heiko

[0] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3742

Powered by blists - more mailing lists