Re: [PATCH] dt-bindings: Add silabs,si5341

[Mike Looijmans]

On 26-06-19 23:24, Stephen Boyd wrote:
> 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?

Actual scenario's.

Clocks are required for FPGA logic, and a some of those do not involve 
software drivers at all.

The GTR transceivers on the Xilinx ZynqMP chips use these clocks for SATA and 
PCIe, but the driver implementation from Xilinx for these is far from mature, 
for example it passes the clock frequency as a PHY parameter and isn't even 
aware of the clk framework at the moment. Xilinx hasn't even attempted 
submitting this 3 year old driver to mainline.


>> 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.
> 

I can see where you going with this, and yes, we definitely should promote 
that drivers should take care of their clock (enable) requirements.

For the case of 'clock-critical', that would serve the purpose quite well 
indeed. It does add the risk of people sprinkling that all over the devicetree.

Short term, I guess the best thing to do here is to remove the "always-on" 
property from my patch.

I'll put the "clock-critical" idea on my list of generic clock patches to 
sneak in on other budgets, it'll be right behind "allow sub-1Hz or fractional 
clock rate accuracy" (yes I actually have a use case for that) and "allow 
frequencies over 4GHz" (the 14GHz clock in the Si5341 luckily isn't available 
on the outside so I can cheat)...