Re: [PATCH RESEND 0/2] Common protected-clocks implementation

[Maxime Ripard]

Hi,

On Tue, Mar 09, 2021 at 09:03:14AM +0100, Rasmus Villemoes wrote:
> On 03/09/2020 06.00, Samuel Holland wrote:
> > Stephen, Maxime,
> > 
> > You previously asked me to implement the protected-clocks property in a
> > driver-independent way:
> > 
> > https://www.spinics.net/lists/arm-kernel/msg753832.html
> > 
> > I provided an implementation 6 months ago, which I am resending now:
> > 
> > https://patchwork.kernel.org/patch/11398629/
> > 
> > Do you have any comments on it?
> 
> I'm also interested [1] in getting something like this supported in a
> generic fashion - i.e., being able to mark a clock as
> protected/critical/whatnot by just adding an appropriate property in the
> clock provider's DT node, but without modifying the driver to opt-in to
> handling it.
> 
> Now, as to this implementation, the commit 48d7f160b1 which added the
> common protected-clocks binding says
> 
>   For example, on some Qualcomm firmwares reading or writing certain clk
>   registers causes the entire system to reboot,
> 
> so I'm not sure handling protected-clocks by translating it to
> CLK_CRITICAL and thus calling prepare/enable on it is the right thing to
> do - clks that behave like above are truly "hands off, kernel", so the
> current driver-specific implementation of simply not registering those
> clocks seems to be the right thing to do - or at least the clk framework
> would need to be taught to not actually call any methods on such
> protected clocks.

The idea to use CLK_CRITICAL was also there to allow any clock to be
marked as protected, and not just the root ones. Indeed, by just
ignoring that clock, the parent clock could end up in a situation where
it doesn't have any (registered) child and thus would be disabled,
disabling the ignored clock as well. Reparenting could cause the same
issue.

Calling clk_prepare_enable just allows the kernel to track that it (and
thus its parent) should never be disabled, ever.

> For my use case, either "hands off kernel" or "make sure this clock is
> enabled" would work since the bootloader anyway enables the clock.

The current protected clocks semantics have been that the clock
shouldn't be disabled by the kernel, but "hands off kernel" clocks imply
a slightly different one. I would expect that the bootloader in this
case wouldn't expect any parent or rate (or phase, or any other
parameter really) change, while it might be ok for some other use cases
(like the one Samuel was trying to address for example).

And even if we wanted the kernel to behave that way (making sure there's
no way to change the rate, parents, phase, etc.), the kernel would have
to have knowledge of it to also propagate that restriction to the whole
chain of parents.

Maxim

Attachment: signature.asc
Description: PGP signature