Date:   Wed, 26 Jun 2019 14:24:08 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Mike Looijmans <mike.looijmans@...ic.nl>
Cc:     "mturquette@...libre.com" <mturquette@...libre.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] dt-bindings: Add silabs,si5341

Sorry, I'm getting through my inbox pile and saw this one.

Quoting Mike Looijmans (2019-04-30 22:46:55)
> On 30-04-19 02:17, Stephen Boyd wrote:
> > 
> > Why can't that driver call clk_prepare_enable()? Is there some sort of
> > assumption that this clk will always be enabled and not have a driver
> > that configures the rate and gates/ungates it?
> 
> Not only clk_prepare_enable(), but the driver could also call clk_set_rate() 
> and clk_set_parent() and the likes, but it doesn't, so that's why there is 
> "assigned-clocks" right?
> 
> There are multiple scenario's, similar to why regulators also have properties 
> like these.
> 
> - The clock is related to hardware that the kernel is not aware of.
> - The clock is for a driver that isn't aware of its clock requirements. It 
> might be an extra clock for an FPGA implemented controller that mimics 
> existing hardware.

Are these hypothetical scenarios or actual scenarios you need to
support?

> 
> I'd also consider patching "assigned-clocks" to call "clk_prepare_enable()", 
> would that make sense, or is it intentional that assigned-clocks doesn't do that?
> 

It's intentional that assigned-clocks doesn't really do anything besides
setup the list of clks to operate on with the rate or parent settings
specified in other properties. We would need to add another property
indicating which clks we want to mark as 'critical' or 'always-on'.

There have been prior discussions where we had developers want to mark
clks with the CLK_IS_CRITICAL flag from DT, but we felt like that was
putting SoC level details into the DT. While that was correct for SoC
specific clk drivers, I can see board designs where it's configurable
and we want to express that a board has some clks that must be enabled
early on and left enabled forever because the hardware engineer has
design the board that way. In this case, marking the clk with the
CLK_IS_CRITICAL flag needs to be done from DT.

In fact, we pretty much already have support for this with
of_clk_detect_critical(). Maybe we should re-use that binding with
'clock-critical' to let clk providers indicate that they have some clks
that should be marked critical once they're registered. We could
probably add another property too that indicates certain clks are
enabled out of the bootloader, similar to the regulator-boot-on
property, but where it takes a clock-cells wide list for the provider
the property is inside of.

We need to be careful though and make sure that clk drivers don't start
putting everything in DT. In your example, it sounds like you have a
consumer driver that wants to make sure the clk is prepared or enabled
when the consumer probes. In this case the prepare/enable calls should
be put directly into the consumer driver so it can manage the clk state.
For the case of rates and parents, it's essentially a oneshot
configuration of the rate or the parents of a clk. We don't need to
"undo" the configuration when the device driver is removed. For prepare
and enable though, we typically want to disable clks when the hardware
is not in use to save power. Adding a property to DT should only be done
to indicate a clk must always be on in this board configuration, not to
avoid calling clk_prepare_enable() in the driver probe.

To put it another way, I'm looking to describe how the board is designed
and to indicate that certain clks are always enabled at power on or are
enabled by the bootloader. Configuration has it's place too, just that
configuration is a oneshot sort of thing that never needs to change at
runtime.

Powered by blists - more mailing lists