On 10/28/21 11:24 PM, Rob Herring wrote:

On Wed, Oct 20, 2021 at 01:06:13PM +0200, Marek Vasut wrote:

quoted

On 10/20/21 12:14 PM, Vaittinen, Matti wrote:
[...]

quoted

I wonder if this really is something specific to ROHM ICs? Do you think
this would warrant a generic, non vendor specific property? I am Ok with
the ROHM specific property too but it just seems to me this might not be
unique to ROHM IC(s).

I imagine we debated the need for a DT property when critical clocks was
added to the kernel.

Have you got some reference to this debate ?

I think something like clk-hog , similar to gpio-hog , would be useful, 
since we could also configure the critical clock frequency in DT.

quoted

quoted

By the way, the very same clk driver where you implemented the property
reading (patch 2/2) is used by few other ROHM PMICs. At least by
BD71837, BD71828, BD71815, BD9576 and BD9573. So the code change here
adds support for this property to all of those PMICs. I wonder if the
property should be mentioned in all of the binding docs... That could be
another argument for making this a generic property and describing it in
clk yaml ;)

Well, just my 10 Cents - I am ok with this change as you presented it
here if you don't think this should be generic one.

I think we need something like gpio-hog, except for clock. Some clk-hog
maybe ? That would be useful not only here, but also for things where some
output generates clock for random stuff which cannot be described in the DT
for whatever reason (like e.g. the SoC is used as a substitute for CPLD XTAL
and the CPLD isn't connected to the SoC in any other way).

The justification given in this patch was for an SoC input which should
get described so that the clock is handled and kept enabled properly.

This is the case I had here, yes. Although I've been running into 
similar requirements repeatedly for almost a decade, I'm surprised 
nobody implemented something like this yet.

The CPLD case would be more interesting, but is there an actual need or
just a possible case?

This is an iMX53 board from 2012 or so, where they figured they don't 
need an XTal for the CPLD because the SoC has this OSC_OUT and that can 
be used to supply clock to the CPLD at just the frequency they need. So 
the SoC is a clock source for the CPLD, and that's all there is to it.

So far I hacked it in the clock driver to keep the clock running at 
specific rate, but that hack has been a thorn in my side for long enough.

You could use the 'protected-clocks' property here. Maybe that's a bit
overloaded between can't access and don't turn off. But what it means is
really up the clock controller.

This does not seem to describe what is needed here, protected-clock are 
used to tell OS not to touch certain clock because they are protected by 
e.g. firmware access restriction, it does not say anything about whether 
the clock are critical. Also, it seems to be a non-generic property only 
for some qualcomm clock driver.

commit 48d7f160b10711f014bf07b574c73452646c9fdd
[...]
dt-bindings: clk: Introduce 'protected-clocks' property

Add a generic clk property for clks which are not intended to be used by
the OS due to security restrictions put in place by firmware. For
example, on some Qualcomm firmwares reading or writing certain clk
registers causes the entire system to reboot, but on other firmwares
reading and writing those same registers is required to make devices
like QSPI work. Rather than adding one-off properties each time a new
set of clks appears to be protected, let's add a generic clk property to
describe any set of clks that shouldn't be touched by the OS. This way
we never need to register the clks or use them in certain firmware
configurations.