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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 09 Mar 02:03 CST 2021, 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.
> 

I can confirm that this is the case. Marking the clocks as critical does
not prevent the kernel from touching these registers so the boards where
this is used doesn't boot with the two patches applied.

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

Our use case is that depending on firmware these platform might handle
some specific clocks in firmware or in the kernel, and in the prior case
security permissions are set up such that these registers are off limit
for the kernel.

Regards,
Bjorn

> Rasmus
> 
> [1]
> https://lore.kernel.org/lkml/20210226141411.2517368-1-linux@rasmusvillemoes.dk/
>