Date:   Thu, 2 Jan 2020 10:45:32 +0100
From:   Maxime Ripard <mripard@...nel.org>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Chen-Yu Tsai <wens@...e.org>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] clk: Implement protected-clocks for all OF clock
 providers

Hi,

(Thanks for that good commit log)

On Mon, Dec 30, 2019 at 01:31:27PM -0600, Samuel Holland wrote:
> This is a generic implementation of the "protected-clocks" property from
> the common clock binding. It 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:
>
>   i)  From the clock core's perspective, hw->init is const.
>
>   ii) 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:
>
>   i)  Clocks with CLK_IS_CRITICAL in their init data are already
>       prepared/enabled, and we don't want to prepare/enable them again.
>       Note that if the clock did not have CLK_SET_RATE_GATE in its init
>       data, it was *not* rate protected during the initial call to
>       clk_core_prepare().  As far as I can tell, none of the other flags
>       affect the internal state of the clock, so they don't need any
>       "parallel reconstruction".
>
>   ii) 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(). I think that the second call
>       in of_clk_init() should be removed, but that would require
>       auditing all users of CLK_OF_DECLARE to ensure they called one of
>       the of_clk_add{,_hw}_provider functions.
>
> 3) It is not clear specifically which flags should be implied by being
> in protected-clocks. I took it to mean "this clock is outside of OS
> control, so don't modify it, and assume it can change at any time".
>
> For that reason, I added the following flags:
>   - CLK_SET_RATE_GATE: prevents clk_set_rate() once prepared
>   - CLK_SET_PARENT_GATE: prevents clk_set_parent() once prepared

I'm not sure the reasoning about the fact that the clock is outside of
OS control and therefore cannot change rate is correct.

Using crust as an example, if you were to all the PRCM clocks handling
code in it, you would have already a number of cases where we would
need to change the rate. I2C bus rate and IR comes to my mind. CPUFreq
is another common example of a setup where the clock itself is outside
of the OS control, but still the OS initiates the rate change.

The RPi also has a PLL to control the whole display block that needs
to be configured depending on the resolution being displayed (and
other things), and is handled by the videocore firmware.

Of course, once the OS has asked for that new rate, the firmware is
entirely free to ignore it as long as it's properly reported to the
clock framework. But completely disabling the rates changes seems a
bit overkill.

>   - CLK_GET_RATE_NOCACHE: allows the rate to change behind the OS's back
>   - CLK_GET_ACCURACY_NOCACHE: ditto for the accuracy

So I was part of the discussions of the protected-clocks stuff a few
years ago and IIRC the discussions were only about the OS not
disabling the clocks.

I guess the arguments are sound though, but at the same time it's
pretty easy (and cheap) to do it at the driver level, and that would
make sense too. So I don't really know what to think here :)

Thanks for starting that discussion!
Maxime

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

Powered by blists - more mailing lists