On Wed, Sep 02, 2020 at 11:00:14PM -0500, Samuel Holland wrote:
> This is a generic implementation of the "protected-clocks" property from
> the common clock binding. It allows firmware to inform the OS about
> clocks that must not be disabled while the OS is running.
>
> This implementation comes with some caveats:
>
> 1) Clocks that have CLK_IS_CRITICAL in their init data are prepared/
> enabled before they are attached to the clock tree. protected-clocks are
> only protected once the clock provider is added, which is generally
> after all of the clocks it provides have been registered. This leaves a
> window of opportunity where something could disable or modify the clock,
> such as a driver running on another CPU, or the clock core itself. There
> is a comment to this effect in __clk_core_init():
>
> /*
> * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
> * don't get accidentally disabled when walking the orphan tree and
> * reparenting clocks
> */
>
> Similarly, these clocks will be enabled after they are first reparented,
> unlike other CLK_IS_CRITICAL clocks. See the comment in
> clk_core_reparent_orphans_nolock():
>
> /*
> * We need to use __clk_set_parent_before() and _after() to
> * to properly migrate any prepare/enable count of the orphan
> * clock. This is important for CLK_IS_CRITICAL clocks, which
> * are enabled during init but might not have a parent yet.
> */
>
> Ideally we could detect protected clocks before they are reparented, but
> there are two problems with that:
>
> a) From the clock core's perspective, hw->init is const.
>
> b) The clock core doesn't see the device_node until __clk_register is
> called on the first clock.
>
> So the only "race-free" way to detect protected-clocks is to do it in
> the middle of __clk_register, between when core->flags is initialized
> and calling __clk_core_init(). That requires scanning the device tree
> again for each clock, which is part of why I didn't do it that way.
>
> 2) __clk_protect needs to be idempotent, for two reasons:
>
> a) Clocks with CLK_IS_CRITICAL in their init data are already
> prepared/enabled, and we don't want to prepare/enable them again.
>
> b) of_clk_set_defaults() is called twice for (at least some) clock
> controllers registered with CLK_OF_DECLARE. It is called first in
> of_clk_add_provider()/of_clk_add_hw_provider() inside clk_init_cb,
> and again afterward in of_clk_init(). The second call in
> of_clk_init() may be unnecessary, but verifying that would require
> auditing all users of CLK_OF_DECLARE to ensure they called one of
> the of_clk_add{,_hw}_provider functions.
>
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
Reviewed-by: Maxime Ripard <mripard@xxxxxxxxxx>
Thanks!
Maxime
Attachment:
signature.asc
Description: PGP signature
