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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Mike Looijmans (2019-06-27 04:38:16)
> 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 think they may have submitted the "fixed rate from PHY parameter"
support. I merged it because I suspected it would help in those rare
cases where an MMIO register is used to express information about the
configuration and the bootloader does nothing to help create a fixed
rate clk in DT.

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

Ok. Will you resend or should I look at the latest binding patch and
remove always-on? I don't see it there so maybe everything is fine.

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

Ok. Good to know it's not as high a priority as these other things.