Message-ID: <155632584222.168659.9675557812377718927@swboyd.mtv.corp.google.com>
Date:   Fri, 26 Apr 2019 17:44:02 -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

Quoting Mike Looijmans (2019-04-25 23:51:15)
> On 26-04-19 01:04, Stephen Boyd wrote:
> > Quoting Mike Looijmans (2019-04-24 02:02:16)
> >> Adds the devicetree bindings for the si5341 driver that supports the
> >> Si5341 and Si5340 chips.
> >>
> >> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
> >> ---
> >>   .../bindings/clock/silabs,si5341.txt          | 141 ++++++++++++++++++
> >>   1 file changed, 141 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5341.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/silabs,si5341.txt b/Documentation/devicetree/bindings/clock/silabs,si5341.txt
> >> new file mode 100644
> >> index 000000000000..1a00dd83100f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/silabs,si5341.txt
> >> @@ -0,0 +1,141 @@
> >> +Binding for Silicon Labs Si5341 and Si5340 programmable i2c clock generator.
> >> +
> >> +Reference
> >> +[1] Si5341 Data Sheet
> >> +    https://www.silabs.com/documents/public/reference-manuals/Si5341-40-D-RM.pdf
> > 
> > Thanks! I also had to look up the pinout in the datasheet, not the
> > reference manual above.
> 
> Now you mention it, this is the "reference manual", not the datasheet. I'll 
> add a reference to that as well.
> 

Cool, thanks.

> 
> >> +
> >> +Optional properties:
> >> +- silabs,pll-m-num, silabs,pll-m-den: Numerator and denominator for PLL
> >> +  feedback divider. Must be such that the PLL output is in the valid range. For
> >> +  example, to create 14GHz from a 48MHz xtal, use m-num=14000 and m-den=48. Only
> >> +  the fraction matters, using 3500 and 12 will deliver the exact same result.
> >> +  If these are not specified, and the PLL is not yet programmed when the driver
> >> +  probes, the PLL will be set to 14GHz.
> > 
> > Can this be done via assigned-clock-rates? Possibly with a table in the
> > clk driver to tell us how to generate those rates.
> 
> The PLL frequency choice determines who'll get jitter and who won't. It's 
> ridiculously accurate too.
> 
> For example, if you need a 26 MHz and a 100 MHz output, there's no solution 
> for the PLL that makes both clocks an integer divider (SI is vague about it, 
> but apparently integer dividers have less jitter on output). Only the enduser 
> can say which clock will get the better quality.
> 
> > 
> >> +- silabs,reprogram: When present, the driver will always assume the device must
> >> +  be initialized, and always performs the soft-reset routine. Since this will
> >> +  temporarily stop all output clocks, don't do this if the chip is generating
> >> +  the CPU clock for example.
> > 
> > Could this be done with the reset framework? It almost sounds like if
> > the clk is a CLK_IS_CRITICAL then we shouldn't do the reset, otherwise
> > we probably should reset the chip when the driver probes. If we don't
> > have a case where it's going to be supplying a critical clk for a long
> > time then perhaps we shouldn't even consider this topic until later.
> 
> The driver can sort of see that the chips is already configured. This tells 
> the driver whether that's expected or just coincidence.
> 
> Maybe it'd be clearer if I reversed the logic and name this 
> "silabs,preprogrammed", which will skip the driver's initialization routine?
> 

Maybe? Is there any way to look at some register to figure out for sure
if it's been pre-programmed or not? Could TOOL_VERSION be used or
ACTIVE_NVM_BANK or DESIGN_ID0-7?

> 
> > 
> >> +
> > 
> > Looks like there is an interrupt pin? So we need an optional interrupts
> > property?  Also, a reset pin, so maybe a 'resets' property. There's also
> > another input pin for 'output enable' which maybe someone wants to use?
> > Plus some other pins to control step up/down of frequency and clock
> > synchronization? Maybe those should be optional gpios, but it probably
> > can wait until later.
> 
> There's indeed an interrupt, that can tell you when a clock stops running.
> 
> The issue here is that supporting all that the chip can do in a driver will 
> take weeks of programming, whereas the added value is next to nothing.
> 
> I assumed the enable, step, and similar pins are for cpu-less operation, since 
> they don't add any functionality (you can do all that through the I2C interface).
> 

I'm not asking for the driver code to be written, just for the
properties to be documented in the binding. I suppose if there isn't
going to be code for them and they're complicated then it might not make
sense to add the properties. All that matters immediately is to get the
required properties correct. If those pins are for cpu-less operation
then I agree it makes sense to not describe them in the binding.

> 
> > 
> >> +==Child nodes==
> >> +
> >> +Each of the clock outputs can be overwritten individually by
> >> +using a child node to the I2C device node. If a child node for a clock
> >> +output is not set, the configuration remains unchanged.
> >> +
> >> +Required child node properties:
> >> +- reg: number of clock output.
> >> +
> >> +Optional child node properties:
> >> +- silabs,format: Output format, see [1], 1=differential, 2=low-power, 4=LVCMOS
> >> +- silabs,common-mode: Output common mode, depends on standard.
> >> +- silabs,amplitude: Output amplitude, depends on standard.
> >> +- silabs,synth-source: Select which multisynth to use for this output
> >> +- silabs,synth-frequency: Sets the frequency for the multisynth connected to
> >> +  this output. This will affect other outputs connected to this multisynth. The
> >> +  setting is applied before silabs,synth-master and clock-frequency.
> >> +- silabs,synth-master: If present, this output is allowed to change the
> >> +  multisynth frequency dynamically.
> > 
> > These above properties look like highly detailed configuration data to
> > let the driver configure the clk output exactly how it's supposed to be
> > configured. Can these properties be rewritten in more high-level terms
> > that a system integrator would understand? Ideally, I shouldn't have to
> > read the datasheet and the driver and then figure out what DT properties
> > need the values from the datasheet in them so that the driver writes
> > them to a particular register. I don't know if that's possible here,
> > because I haven't read the driver or the datasheet too closely yet, but
> > that should be the goal.
> 
> The datasheet is not very helpful in this regard. Silabs just assumes you'll 
> use their clockbuilder software for writing these values, which is how we got 
> to the "LVDS 3v3" values.

I hope that can be determined by looking at vdd<N>-supply voltages?

> 
> I could put in a table of "common" values, so that you can say:
> 
> silabs,output-standard = "lvds";
> 
> And then use the "raw" properties to expand or override on that.
> 
> Extra defines might help, e.g.:
> 
> silabs,format = <SI5341_FORMAT_DIFFERENTIAL>;

I suppose you'll need to reverse engineer the clock builder software to
figure out why a SI5341_FORMAT_DIFFERENTIAL would be specified instead
of some other value. Ideally we don't need any of these vendor specific
properties and the drivers using these clks can ask the clk framework to
configure these properties, or we need to look at making more properties
like 'assigned-clock-parents' that lets us configure things generically.

> 
> >> +- clock-frequency: Sets a default frequency for this output.
> > 
> > Why not use assigned-clock-rates?
> 
> Suppose you want to have a synchronized audio and video clock. The audio needs 
> a 1536000 Hz clock and video wants 36864000. The clocks must have the same 
> parent (so they don't drift).
> 
> Without pre-specifying the clocks, the video might probe first and change the 
> synth to run at 73728000. The audio clock will then run at some undefined 
> frequency until the driver probes. The audio hardware may not like that.
> 
> If the audio probes first, it could program the synth to 3072000 for example. 
> Then when the video probes, it needs to re-program the synth, and then the 
> audio must adapt its divider.

I think you're describing a need to call clk_set_rate_exclusive() on
certain clks provided by this node? But I'm lost how this property is
solving that problem.

> 
> Another example, a serdes needs a 100MHz clock. This could set the synth to 
> 200MHz, and that's fine. Another device needs a 200MHz clock. This can share 
> the same synth, by programming it to 400MHz using a /4 and /2 divider. But if 
> that 100MHz serdes is already up and running, changing the synth config will 
> cause the serdes to lose its lock and result in transfer errors. This would 
> not happen if the 200MHz device happens to probe before that serdes. The 
> solution provided by the driver is that you can pre-set the synth to 400 and 
> then both drivers can probe in any order they like.
> 
> While the driver can change the muxing and dividers and multipliers, there's 
> more to it that just "gimme a frequency". So I let the user describe a 
> (partial) solution in the devicetree.

How is this different from using assigned-clock-rates with the
"consumer" being the clk provider node and the rate being 400MHz?

> 
> >> +- always-on: Immediately and permanently enable this output. Particulary
> >> +  useful when combined with assigned-clocks, since that does not prepare clocks.
> > 
> > Looks like you want CLK_IS_CRITICAL in DT. We've had that discussion
> > before but maybe we should revisit it here and add a way to indicate
> > that some clk should never be turned off instead of assuming that we
> > can do this from C code all the time.
> 
> My issue was that assigned-clocks does not call clk_prepare. If the clock is 
> not running, assigned-clocks will not turn it on (at least, that is the case 
> on the 4.14 kernel I tested this on), it apparently only prevents it from 
> being turned off by marking it as "in use". This just provides a way to use 
> assigned-clocks.
> 

Do you want the clks to always be prepared and enabled? What use-case is
this? It still looks like CLK_IS_CRITICAL flag needs to be expressed in
DT here.

Powered by blists - more mailing lists