[PATCH] dt-bindings: Add silabs,si5341

[PATCH] dt-bindings: Add silabs,si5341

[Mike Looijmans]

Adds the devicetree bindings for the si5341 driver that supports the
Si5341 and Si5340 chips.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.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
+
+The Si5341 and Si5340 are programmable i2c clock generators with up to 10 output
+clocks. The chip contains a PLL that sources 5 (or 4) multisynth clocks, which
+in turn can be directed to any of the 10 (or 4) outputs through a divider.
+The internal structure of the clock generators can be found in [1].
+
+The driver can be used in "as is" mode, reading the current settings from the
+chip at boot, in case you have a (pre-)programmed device. If the PLL is not
+configured when the driver probes, it assumes the driver must fully initialize
+it.
+
+The device type, speed grade and revision are determined runtime by probing.
+
+The driver currently only supports XTAL input mode, and does not support any
+fancy input configurations. They can still be programmed into the chip and
+the driver will leave them "as is".
+
+==I2C device node==
+
+Required properties:
+- compatible: shall be one of the following: "silabs,si5341", "silabs,si5340"
+- reg: i2c device address, usually 0x74
+- #clock-cells: from common clock binding; shall be set to 1.
+- clocks: from common clock binding; list of parent clock
+  handles, shall be xtal reference clock. Usually a fixed clock.
+- clock-names: Shall be "xtal".
+- #address-cells: shall be set to 1.
+- #size-cells: shall be set to 0.
+
+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.
+- 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.
+
+==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.
+- clock-frequency: Sets a default frequency for this output.
+- always-on: Immediately and permanently enable this output. Particulary
+  useful when combined with assigned-clocks, since that does not prepare clocks.
+
+==Example==
+
+/* 48MHz reference crystal */
+ref48: ref48M {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <48000000>;
+};
+
+i2c-master-node {
+
+	/* Programmable clock (for logic) */
+	si5341: clock-generator@74 {
+		reg = <0x74>;
+		compatible = "silabs,si5341";
+		#clock-cells = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&ref48>;
+		clock-names = "xtal";
+
+		silabs,pll-m-num = <14000>; /* PLL at 14.0 GHz */
+		silabs,pll-m-den = <48>;
+		silabs,reprogram; /* Chips are not programmed, always reset */
+
+		/*
+		 * Output 0 configuration:
+		 *  LVDS 3v3
+		 *  Source from Multisynth 3
+		 *  Use 600MHz synth frequency, and generate 100MHz from that
+		 *  Always keep this clock running
+		 */
+		out0 {
+			/* refclk0 for PS-GT, usually for SATA or PCIe */
+			reg = <0>;
+			silabs,format = <1>; /* LVDS 3v3 */
+			silabs,common-mode = <3>;
+			silabs,amplitude = <3>;
+			silabs,synth-source = <3>; /* Multisynth 3 */
+			silabs,synth-frequency = <600000000>;
+			silabs,synth-master;
+			clock-frequency = <10000000>;
+			always-on;
+		};
+
+		/*
+		 * Output 6 configuration:
+		 *  LVDS 1v8
+		 */
+		out6 {
+			/* FPGA clock 200MHz */
+			reg = <6>;
+			silabs,format = <1>; /* LVDS 1v8 */
+			silabs,common-mode = <13>;
+			silabs,amplitude = <3>;
+		};
+
+		/*
+		 * Output 8 configuration:
+		 *  HCSL 3v3
+		 *  run at 100MHz
+		 */
+		out8 {
+			reg = <8>;
+			silabs,format = <2>;
+			silabs,common-mode = <11>;
+			silabs,amplitude = <3>;
+			silabs,synth-source = <0>;
+			clock-frequency = <100000000>;
+		};
+	};
+};
-- 
2.17.1

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

[Stephen Boyd]

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

> +
> +The Si5341 and Si5340 are programmable i2c clock generators with up to 10 output
> +clocks. The chip contains a PLL that sources 5 (or 4) multisynth clocks, which
> +in turn can be directed to any of the 10 (or 4) outputs through a divider.
> +The internal structure of the clock generators can be found in [1].
> +
> +The driver can be used in "as is" mode, reading the current settings from the
> +chip at boot, in case you have a (pre-)programmed device. If the PLL is not
> +configured when the driver probes, it assumes the driver must fully initialize
> +it.
> +
> +The device type, speed grade and revision are determined runtime by probing.
> +
> +The driver currently only supports XTAL input mode, and does not support any
> +fancy input configurations. They can still be programmed into the chip and
> +the driver will leave them "as is".
> +
> +==I2C device node==
> +
> +Required properties:
> +- compatible: shall be one of the following: "silabs,si5341", "silabs,si5340"
> +- reg: i2c device address, usually 0x74
> +- #clock-cells: from common clock binding; shall be set to 1.
> +- clocks: from common clock binding; list of parent clock
> +  handles, shall be xtal reference clock. Usually a fixed clock.

Is there only one possible clk parent? Looks like there's an optional
xtal on the XA/XB pins and then up to three more input clks on IN0/1/2.
So shouldn't this list all of those and then indicate that at least one
should be specified at all times?

> +- clock-names: Shall be "xtal".

This should include the other clk inputs?

> +- #address-cells: shall be set to 1.
> +- #size-cells: shall be set to 0.

I'd expect to see all the input voltage supplies here too.

	vdd-supply
	vdda-supply
	vdds-supply
	vdd0-supply
	vdd1-supply
	vdd2-supply
	vdd3-supply
	vdd4-supply
	vdd5-supply
	vdd6-supply
	vdd7-supply
	vdd8-supply
	vdd9-supply

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

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

> +

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.

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

> +- clock-frequency: Sets a default frequency for this output.

Why not use assigned-clock-rates?

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

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

[Mike Looijmans]

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

>> +
>> +The Si5341 and Si5340 are programmable i2c clock generators with up to 10 output
>> +clocks. The chip contains a PLL that sources 5 (or 4) multisynth clocks, which
>> +in turn can be directed to any of the 10 (or 4) outputs through a divider.
>> +The internal structure of the clock generators can be found in [1].
>> +
>> +The driver can be used in "as is" mode, reading the current settings from the
>> +chip at boot, in case you have a (pre-)programmed device. If the PLL is not
>> +configured when the driver probes, it assumes the driver must fully initialize
>> +it.
>> +
>> +The device type, speed grade and revision are determined runtime by probing.
>> +
>> +The driver currently only supports XTAL input mode, and does not support any
>> +fancy input configurations. They can still be programmed into the chip and
>> +the driver will leave them "as is".
>> +
>> +==I2C device node==
>> +
>> +Required properties:
>> +- compatible: shall be one of the following: "silabs,si5341", "silabs,si5340"
>> +- reg: i2c device address, usually 0x74
>> +- #clock-cells: from common clock binding; shall be set to 1.
>> +- clocks: from common clock binding; list of parent clock
>> +  handles, shall be xtal reference clock. Usually a fixed clock.
> 
> Is there only one possible clk parent? Looks like there's an optional
> xtal on the XA/XB pins and then up to three more input clks on IN0/1/2.
> So shouldn't this list all of those and then indicate that at least one
> should be specified at all times?
> 
>> +- clock-names: Shall be "xtal".
> 
> This should include the other clk inputs?

Some day maybe. That's what I meant when I wrote "does not support any fancy 
input configurations".

The input config is horrendously complex. We have never used anything but just 
the xtal input, and I think that goes for 99.9% of the use cases for this chip.

I already went way over budget with this one, my first intention was to write 
a driver that takes a firmware blob from the "clockbuilder" software, but 
while writing it I discovered that the whole damn thing could easily be 
controlled completely without it.

> 
>> +- #address-cells: shall be set to 1.
>> +- #size-cells: shall be set to 0.
> 
> I'd expect to see all the input voltage supplies here too.
> 
> 	vdd-supply
> 	vdda-supply
> 	vdds-supply
> 	vdd0-supply
> 	vdd1-supply
> 	vdd2-supply
> 	vdd3-supply
> 	vdd4-supply
> 	vdd5-supply
> 	vdd6-supply
> 	vdd7-supply
> 	vdd8-supply
> 	vdd9-supply

I'll look into it. Might be useful for some register settings.

>> +
>> +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?


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


> 
>> +==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 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>;

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

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.

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

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

[Stephen Boyd]

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

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

[Mike Looijmans]

On 27-04-19 02:44, Stephen Boyd wrote:
> 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@topic.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?

I've experimentally determined that TOOL_VERSION and DESIGN_IDx 
apparently get filled with zeroes by the clockbuilder anyway.

ACTIVE_NVM_BANK works reliably for self-programmed chips.

The flag is about "is this chip under full kernel control, or is it 
generating clocks the kernel doesn't know about (e.g. for realtime cores 
or FPGA logic)".


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

Clear, probably the interrupt makes sense, and so do the vddX supplies.

>>>> +==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?

Not really, and when asked for a bit more detail, Silabs just says to 
use the excellent developer friendly clockbuilder software that almost 
runs on almost any platform.

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

These properties are about how you soldered things together, but yeah, 
if the clock outputs go to expansion slots, some driver or devicetree 
fragment control would be desirable, so you can switch the clock mode 
from differential to single ended for example.


>>>> +- 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?

Having slept on it, I think you're right, and assigned-clock-* should be 
able to provide this.

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

The use case is pretty simple, it's just to enable the clock. Or in this 
case, "prepare" it, because the I2C client needs to be able to sleep and 
all actions must be done in the prepare part.

Suppose I use the si5341 to generate a 26MHz clock that would normally 
be provides by a hardware oscillator. The driver itself doesn't have any 
clock properties to set. Then I'd put into that device's devicetree node 
the following:

assigned-clocks = <&si5341 2>;
assigned-clock-rates = <26000000>;

When the system boots, the clock framework indeed calls 
"set_rate(26000000)" before that driver probes, but it never calls 
"clk_prepare", so the clock's frequency is set okay but the clock won't 
be running and the device won't function.

-- 
Mike Looijmans

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

[Stephen Boyd]

Quoting Mike Looijmans (2019-04-27 02:42:56)
> On 27-04-19 02:44, Stephen Boyd wrote:
> > Quoting Mike Looijmans (2019-04-25 23:51:15)
> >> On 26-04-19 01:04, Stephen Boyd wrote:
> >>>> +
> >>>> +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.

Right. So maybe we make tables of rates and put it in the driver and
keep adding code in there? I'm worried about having these properties in
DT and then having to work around them later on by "fixing" the DT. If
it's only in the driver then we're able to tweak the values to get
better jitter numbers, etc.

> >>
> >>>
> >>>> +- 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?
> 
> I've experimentally determined that TOOL_VERSION and DESIGN_IDx 
> apparently get filled with zeroes by the clockbuilder anyway.
> 
> ACTIVE_NVM_BANK works reliably for self-programmed chips.
> 
> The flag is about "is this chip under full kernel control, or is it 
> generating clocks the kernel doesn't know about (e.g. for realtime cores 
> or FPGA logic)".
> 

Alright.

> 
> 
> >>>> +==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?
> 
> Not really, and when asked for a bit more detail, Silabs just says to 
> use the excellent developer friendly clockbuilder software that almost 
> runs on almost any platform.
> 
> >> 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.
> 
> These properties are about how you soldered things together, but yeah, 
> if the clock outputs go to expansion slots, some driver or devicetree 
> fragment control would be desirable, so you can switch the clock mode 
> from differential to single ended for example.

Hmm.. Perhaps we need a clk_set_mode() API? Possibly be an internal API
that lets the DT configure the mode to be differential or single ended?
Nobody has required this so far so it's very rare, but I'd like to see
the properties become standard instead of vendor specific if possible.

> 
> 
> >>>> +- 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.
> 
> The use case is pretty simple, it's just to enable the clock. Or in this 
> case, "prepare" it, because the I2C client needs to be able to sleep and 
> all actions must be done in the prepare part.
> 
> Suppose I use the si5341 to generate a 26MHz clock that would normally 
> be provides by a hardware oscillator. The driver itself doesn't have any 
> clock properties to set. Then I'd put into that device's devicetree node 
> the following:
> 
> assigned-clocks = <&si5341 2>;
> assigned-clock-rates = <26000000>;
> 
> When the system boots, the clock framework indeed calls 
> "set_rate(26000000)" before that driver probes, but it never calls 
> "clk_prepare", so the clock's frequency is set okay but the clock won't 
> be running and the device won't function.

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?

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

[Mike Looijmans]

On 30-04-19 02:17, Stephen Boyd wrote:
> Quoting Mike Looijmans (2019-04-27 02:42:56)
>> On 27-04-19 02:44, Stephen Boyd wrote:
>>> Quoting Mike Looijmans (2019-04-25 23:51:15)
>>>> On 26-04-19 01:04, Stephen Boyd wrote:
>>>>>> +
>>>>>> +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.
> 
> Right. So maybe we make tables of rates and put it in the driver and
> keep adding code in there? I'm worried about having these properties in
> DT and then having to work around them later on by "fixing" the DT. If
> it's only in the driver then we're able to tweak the values to get
> better jitter numbers, etc.

Programming the main PLL is easy, no tables required.

It's all user choice, that's the issue here. Drivers themselves cannot make 
this decision.

(user = the person doing board support, writing the devicetree and kernel 
config for example)

In my example, the chip has no issue synthesizing both 26 and 100 MHz. 
Depending on the main PLL setting, one may have more jitter than the other. If 
the PLL is set to 14.0 GHz, the 100 MHz clock will be of better quality, while 
if the PLL is set to 13.624 GHz (an even multiple of 26), the 26 MHz will have 
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?
>>
>> I've experimentally determined that TOOL_VERSION and DESIGN_IDx
>> apparently get filled with zeroes by the clockbuilder anyway.
>>
>> ACTIVE_NVM_BANK works reliably for self-programmed chips.
>>
>> The flag is about "is this chip under full kernel control, or is it
>> generating clocks the kernel doesn't know about (e.g. for realtime cores
>> or FPGA logic)".
>>
> 
> Alright.
> 
>>
>>
>>>>>> +==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?
>>
>> Not really, and when asked for a bit more detail, Silabs just says to
>> use the excellent developer friendly clockbuilder software that almost
>> runs on almost any platform.
>>
>>>> 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.
>>
>> These properties are about how you soldered things together, but yeah,
>> if the clock outputs go to expansion slots, some driver or devicetree
>> fragment control would be desirable, so you can switch the clock mode
>> from differential to single ended for example.
> 
> Hmm.. Perhaps we need a clk_set_mode() API? Possibly be an internal API
> that lets the DT configure the mode to be differential or single ended?
> Nobody has required this so far so it's very rare, but I'd like to see
> the properties become standard instead of vendor specific if possible.
> 
>>
>>
>>>>>> +- 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.
>>
>> The use case is pretty simple, it's just to enable the clock. Or in this
>> case, "prepare" it, because the I2C client needs to be able to sleep and
>> all actions must be done in the prepare part.
>>
>> Suppose I use the si5341 to generate a 26MHz clock that would normally
>> be provides by a hardware oscillator. The driver itself doesn't have any
>> clock properties to set. Then I'd put into that device's devicetree node
>> the following:
>>
>> assigned-clocks = <&si5341 2>;
>> assigned-clock-rates = <26000000>;
>>
>> When the system boots, the clock framework indeed calls
>> "set_rate(26000000)" before that driver probes, but it never calls
>> "clk_prepare", so the clock's frequency is set okay but the clock won't
>> be running and the device won't function.
> 
> 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.

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?

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

[Rob Herring]

On Wed, Apr 24, 2019 at 11:02:16AM +0200, Mike Looijmans wrote:
> Adds the devicetree bindings for the si5341 driver that supports the

Bindings are for h/w, not a driver.

Perhaps 'dt-bindings: clock: ...' to give a bit more clue what this is 
in the subject.

> Si5341 and Si5340 chips.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.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
> +
> +The Si5341 and Si5340 are programmable i2c clock generators with up to 10 output
> +clocks. The chip contains a PLL that sources 5 (or 4) multisynth clocks, which
> +in turn can be directed to any of the 10 (or 4) outputs through a divider.
> +The internal structure of the clock generators can be found in [1].
> +
> +The driver can be used in "as is" mode, reading the current settings from the
> +chip at boot, in case you have a (pre-)programmed device. If the PLL is not
> +configured when the driver probes, it assumes the driver must fully initialize
> +it.
> +
> +The device type, speed grade and revision are determined runtime by probing.
> +
> +The driver currently only supports XTAL input mode, and does not support any
> +fancy input configurations. They can still be programmed into the chip and
> +the driver will leave them "as is".
> +
> +==I2C device node==
> +
> +Required properties:
> +- compatible: shall be one of the following: "silabs,si5341", "silabs,si5340"

One per line please.

> +- reg: i2c device address, usually 0x74
> +- #clock-cells: from common clock binding; shall be set to 1.
> +- clocks: from common clock binding; list of parent clock
> +  handles, shall be xtal reference clock. Usually a fixed clock.

More indentation on the continued lines would help readability.

> +- clock-names: Shall be "xtal".
> +- #address-cells: shall be set to 1.
> +- #size-cells: shall be set to 0.
> +
> +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.
> +- 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.

I'm not too sure about this one. Seems like if you have child nodes, 
then you should re-program. If you don't then you don't re-program.

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

Possible values?

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

Boolean?

> +- clock-frequency: Sets a default frequency for this output.

range?

> +- always-on: Immediately and permanently enable this output. Particulary

Particularly

> +  useful when combined with assigned-clocks, since that does not prepare clocks.
> +
> +==Example==
> +
> +/* 48MHz reference crystal */
> +ref48: ref48M {
> +	compatible = "fixed-clock";
> +	#clock-cells = <0>;
> +	clock-frequency = <48000000>;
> +};
> +
> +i2c-master-node {
> +
> +	/* Programmable clock (for logic) */
> +	si5341: clock-generator@74 {
> +		reg = <0x74>;
> +		compatible = "silabs,si5341";
> +		#clock-cells = <1>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&ref48>;
> +		clock-names = "xtal";
> +
> +		silabs,pll-m-num = <14000>; /* PLL at 14.0 GHz */
> +		silabs,pll-m-den = <48>;
> +		silabs,reprogram; /* Chips are not programmed, always reset */
> +
> +		/*
> +		 * Output 0 configuration:
> +		 *  LVDS 3v3
> +		 *  Source from Multisynth 3
> +		 *  Use 600MHz synth frequency, and generate 100MHz from that
> +		 *  Always keep this clock running
> +		 */
> +		out0 {

clock@0

With a reg property, you should have a unit address.

> +			/* refclk0 for PS-GT, usually for SATA or PCIe */
> +			reg = <0>;
> +			silabs,format = <1>; /* LVDS 3v3 */
> +			silabs,common-mode = <3>;
> +			silabs,amplitude = <3>;
> +			silabs,synth-source = <3>; /* Multisynth 3 */
> +			silabs,synth-frequency = <600000000>;
> +			silabs,synth-master;
> +			clock-frequency = <10000000>;
> +			always-on;
> +		};
> +
> +		/*
> +		 * Output 6 configuration:
> +		 *  LVDS 1v8
> +		 */
> +		out6 {
> +			/* FPGA clock 200MHz */
> +			reg = <6>;
> +			silabs,format = <1>; /* LVDS 1v8 */
> +			silabs,common-mode = <13>;
> +			silabs,amplitude = <3>;
> +		};
> +
> +		/*
> +		 * Output 8 configuration:
> +		 *  HCSL 3v3
> +		 *  run at 100MHz
> +		 */
> +		out8 {
> +			reg = <8>;
> +			silabs,format = <2>;
> +			silabs,common-mode = <11>;
> +			silabs,amplitude = <3>;
> +			silabs,synth-source = <0>;
> +			clock-frequency = <100000000>;
> +		};
> +	};
> +};
> -- 
> 2.17.1
> 

[PATCH v2] dt-bindings: clock: Add silabs,si5341

[Mike Looijmans]

Adds the devicetree bindings for the Si5341 and Si5340 chips from
Silicon Labs. These are multiple-input multiple-output clock
synthesizers.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
v2: Add data sheet reference.
    Restructured to enable use of "assigned-clock*" properties to set
    up both outputs and internal synthesizers.
    Nicer indentation.
    Updated subject line and body of commit message.
    If these bindings are (mostly) acceptable, I'll post an updated
    driver patch v2 to implement these changes.

 .../bindings/clock/silabs,si5341.txt          | 187 ++++++++++++++++++
 1 file changed, 187 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..6086dfcaeecf
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/silabs,si5341.txt
@@ -0,0 +1,187 @@
+Binding for Silicon Labs Si5341 and Si5340 programmable i2c clock generator.
+
+Reference
+[1] Si5341 Data Sheet
+    https://www.silabs.com/documents/public/data-sheets/Si5341-40-D-DataSheet.pdf
+[2] Si5341 Reference Manual
+    https://www.silabs.com/documents/public/reference-manuals/Si5341-40-D-RM.pdf
+
+The Si5341 and Si5340 are programmable i2c clock generators with up to 10 output
+clocks. The chip contains a PLL that sources 5 (or 4) multisynth clocks, which
+in turn can be directed to any of the 10 (or 4) outputs through a divider.
+The internal structure of the clock generators can be found in [2].
+
+The driver can be used in "as is" mode, reading the current settings from the
+chip at boot, in case you have a (pre-)programmed device. If the PLL is not
+configured when the driver probes, it assumes the driver must fully initialize
+it.
+
+The device type, speed grade and revision are determined runtime by probing.
+
+The driver currently only supports XTAL input mode, and does not support any
+fancy input configurations. They can still be programmed into the chip and
+the driver will leave them "as is".
+
+==I2C device node==
+
+Required properties:
+- compatible: shall be one of the following:
+	"silabs,si5340" - Si5340 A/B/C/D
+	"silabs,si5341" - Si5341 A/B/C/D
+- reg: i2c device address, usually 0x74
+- #clock-cells: from common clock binding; shall be set to 2.
+	The first value is "0" for outputs, "1" for synthesizers.
+	The second value is the output or synthesizer index.
+- clocks: from common clock binding; list of parent clock  handles,
+	corresponding to inputs. Use a fixed clock for the "xtal" input.
+	At least one must be present.
+- clock-names: One of: "xtal", "in0", "in1", "in2"
+- vdd-supply: Regulator node for VDD
+
+Optional properties:
+- vdda-supply: Regulator node for VDDA
+- vdds-supply: Regulator node for VDDS
+- 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.
+- 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.
+- interrupts: Interrupt for INTRb pin.
+
+== Child nodes: Synthesizers ==
+
+In order to refer to the internal synthesizers, there can be a child node named
+"synthesizers".
+
+Required synthesizers node properties:
+- #address-cells: shall be set to 1.
+- #size-cells: shall be set to 0.
+
+Each child of this node corresponds to a multisynth in the Si534X chip. This
+allows the synthesizer to be referred to with assigned-clocks.
+
+Required child node properties:
+- reg: synthesizer index in range 0..4 for Si5341 and 0..3 for Si5340.
+
+== Child nodes: Outputs ==
+
+The child node "outputs" lists the output clocks.
+
+Required outputs node properties:
+- #address-cells: shall be set to 1.
+- #size-cells: shall be set to 0.
+
+Each of the clock outputs can be overwritten individually by
+using a child node to the outputs child 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:
+- vdd-supply: Regulator node for VDD for this output. The driver selects default
+	values for common-mode and amplitude based on the voltage.
+- silabs,format: Output format, one of:
+	1 = differential (defaults to LVDS levels)
+	2 = low-power (defaults to HCSL levels)
+	4 = LVCMOS
+- silabs,common-mode: Manually overide output common mode, see [2] for values
+- silabs,amplitude: Manually override output amplitude, see [2] for values
+- silabs,synth-master: boolean. If present, this output is allowed to change the
+	multisynth frequency dynamically.
+- silabs,disable-state : clock output disable state, shall be
+	0 = clock output is driven LOW when disabled
+	1 = clock output is driven HIGH when disabled
+
+==Example==
+
+/* 48MHz reference crystal */
+ref48: ref48M {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <48000000>;
+};
+
+i2c-master-node {
+	/* Programmable clock (for logic) */
+	si5341: clock-generator@74 {
+		reg = <0x74>;
+		compatible = "silabs,si5341";
+		#clock-cells = <2>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&ref48>;
+		clock-names = "xtal";
+
+		silabs,pll-m-num = <14000>; /* PLL at 14.0 GHz */
+		silabs,pll-m-den = <48>;
+		silabs,reprogram; /* Chips are not programmed, always reset */
+
+		synthesizers {
+			synth@2 {
+				reg = <2>;
+			};
+		};
+
+		outputs {
+			out@0 {
+				reg = <0>;
+				silabs,format = <1>; /* LVDS 3v3 */
+				silabs,common-mode = <3>;
+				silabs,amplitude = <3>;
+				silabs,synth-master;
+			};
+
+			/*
+			 * Output 6 configuration:
+			 *  LVDS 1v8
+			 */
+			out@6 {
+				reg = <6>;
+				silabs,format = <1>; /* LVDS 1v8 */
+				silabs,common-mode = <13>;
+				silabs,amplitude = <3>;
+			};
+
+			/*
+			 * Output 8 configuration:
+			 *  HCSL 3v3
+			 */
+			out@8 {
+				reg = <8>;
+				silabs,format = <2>;
+				silabs,common-mode = <11>;
+				silabs,amplitude = <3>;
+			};
+		};
+	};
+};
+
+some-video-node {
+	/* Standard clock bindings */
+	clock-names = "pixel";
+	clocks = <&si5341 0 7>; /* Output 7 */
+
+	/* Set output 7 to use syntesizer 3 as its parent */
+	assigned-clocks = <&si5341 0 7>, <&si5341 1 3>;
+	assigned-clock-parents = <&si5341 1 3>;
+	/* Set output 7 to 148.5 MHz using a synth frequency of 594 MHz */
+	assigned-clock-rates = <148500000>, <594000000>;
+};
+
+some-audio-node {
+	clock-names = "i2s-clk";
+	clocks = <&si5341 0 0>;
+	/*
+	 * since output 0 is a synth-master, the synth will be automatically set
+	 * to an appropriate frequency when the audio driver requests another
+	 * frequency. We give control over synth 2 to this output here.
+	 */
+	assigned-clocks = <&si5341 0 0>;
+	assigned-clock-parents = <&si5341 1 2>;
+};
-- 
2.17.1

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

[Rob Herring]

On Tue, May 07, 2019 at 04:04:13PM +0200, Mike Looijmans wrote:
> Adds the devicetree bindings for the Si5341 and Si5340 chips from
> Silicon Labs. These are multiple-input multiple-output clock
> synthesizers.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> v2: Add data sheet reference.
>     Restructured to enable use of "assigned-clock*" properties to set
>     up both outputs and internal synthesizers.
>     Nicer indentation.
>     Updated subject line and body of commit message.
>     If these bindings are (mostly) acceptable, I'll post an updated
>     driver patch v2 to implement these changes.
> 
>  .../bindings/clock/silabs,si5341.txt          | 187 ++++++++++++++++++
>  1 file changed, 187 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..6086dfcaeecf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/silabs,si5341.txt
> @@ -0,0 +1,187 @@
> +Binding for Silicon Labs Si5341 and Si5340 programmable i2c clock generator.
> +
> +Reference
> +[1] Si5341 Data Sheet
> +    https://www.silabs.com/documents/public/data-sheets/Si5341-40-D-DataSheet.pdf
> +[2] Si5341 Reference Manual
> +    https://www.silabs.com/documents/public/reference-manuals/Si5341-40-D-RM.pdf
> +
> +The Si5341 and Si5340 are programmable i2c clock generators with up to 10 output
> +clocks. The chip contains a PLL that sources 5 (or 4) multisynth clocks, which
> +in turn can be directed to any of the 10 (or 4) outputs through a divider.
> +The internal structure of the clock generators can be found in [2].
> +
> +The driver can be used in "as is" mode, reading the current settings from the
> +chip at boot, in case you have a (pre-)programmed device. If the PLL is not
> +configured when the driver probes, it assumes the driver must fully initialize
> +it.
> +
> +The device type, speed grade and revision are determined runtime by probing.
> +
> +The driver currently only supports XTAL input mode, and does not support any
> +fancy input configurations. They can still be programmed into the chip and
> +the driver will leave them "as is".
> +
> +==I2C device node==
> +
> +Required properties:
> +- compatible: shall be one of the following:
> +	"silabs,si5340" - Si5340 A/B/C/D
> +	"silabs,si5341" - Si5341 A/B/C/D
> +- reg: i2c device address, usually 0x74
> +- #clock-cells: from common clock binding; shall be set to 2.
> +	The first value is "0" for outputs, "1" for synthesizers.
> +	The second value is the output or synthesizer index.
> +- clocks: from common clock binding; list of parent clock  handles,
> +	corresponding to inputs. Use a fixed clock for the "xtal" input.
> +	At least one must be present.
> +- clock-names: One of: "xtal", "in0", "in1", "in2"
> +- vdd-supply: Regulator node for VDD
> +
> +Optional properties:
> +- vdda-supply: Regulator node for VDDA
> +- vdds-supply: Regulator node for VDDS
> +- 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.
> +- 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.
> +- interrupts: Interrupt for INTRb pin.
> +
> +== Child nodes: Synthesizers ==
> +
> +In order to refer to the internal synthesizers, there can be a child node named
> +"synthesizers".
> +
> +Required synthesizers node properties:
> +- #address-cells: shall be set to 1.
> +- #size-cells: shall be set to 0.
> +
> +Each child of this node corresponds to a multisynth in the Si534X chip. This
> +allows the synthesizer to be referred to with assigned-clocks.

Why is this? It doesn't seem to me that you need this in DT. You can 
define the clock ID to refer to it in assigned-clocks without these 
nodes in DT.

> +
> +Required child node properties:
> +- reg: synthesizer index in range 0..4 for Si5341 and 0..3 for Si5340.
> +
> +== Child nodes: Outputs ==
> +
> +The child node "outputs" lists the output clocks.
> +
> +Required outputs node properties:
> +- #address-cells: shall be set to 1.
> +- #size-cells: shall be set to 0.
> +
> +Each of the clock outputs can be overwritten individually by
> +using a child node to the outputs child 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:
> +- vdd-supply: Regulator node for VDD for this output. The driver selects default
> +	values for common-mode and amplitude based on the voltage.
> +- silabs,format: Output format, one of:
> +	1 = differential (defaults to LVDS levels)
> +	2 = low-power (defaults to HCSL levels)
> +	4 = LVCMOS
> +- silabs,common-mode: Manually overide output common mode, see [2] for values

s/overide/override/

> +- silabs,amplitude: Manually override output amplitude, see [2] for values
> +- silabs,synth-master: boolean. If present, this output is allowed to change the
> +	multisynth frequency dynamically.
> +- silabs,disable-state : clock output disable state, shall be
> +	0 = clock output is driven LOW when disabled
> +	1 = clock output is driven HIGH when disabled
> +
> +==Example==
> +
> +/* 48MHz reference crystal */
> +ref48: ref48M {
> +	compatible = "fixed-clock";
> +	#clock-cells = <0>;
> +	clock-frequency = <48000000>;
> +};
> +
> +i2c-master-node {
> +	/* Programmable clock (for logic) */
> +	si5341: clock-generator@74 {
> +		reg = <0x74>;
> +		compatible = "silabs,si5341";
> +		#clock-cells = <2>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&ref48>;
> +		clock-names = "xtal";
> +
> +		silabs,pll-m-num = <14000>; /* PLL at 14.0 GHz */
> +		silabs,pll-m-den = <48>;
> +		silabs,reprogram; /* Chips are not programmed, always reset */
> +
> +		synthesizers {
> +			synth@2 {
> +				reg = <2>;
> +			};
> +		};
> +
> +		outputs {
> +			out@0 {
> +				reg = <0>;
> +				silabs,format = <1>; /* LVDS 3v3 */
> +				silabs,common-mode = <3>;
> +				silabs,amplitude = <3>;
> +				silabs,synth-master;
> +			};
> +
> +			/*
> +			 * Output 6 configuration:
> +			 *  LVDS 1v8
> +			 */
> +			out@6 {
> +				reg = <6>;
> +				silabs,format = <1>; /* LVDS 1v8 */
> +				silabs,common-mode = <13>;
> +				silabs,amplitude = <3>;
> +			};
> +
> +			/*
> +			 * Output 8 configuration:
> +			 *  HCSL 3v3
> +			 */
> +			out@8 {
> +				reg = <8>;
> +				silabs,format = <2>;
> +				silabs,common-mode = <11>;
> +				silabs,amplitude = <3>;
> +			};
> +		};
> +	};
> +};
> +
> +some-video-node {
> +	/* Standard clock bindings */
> +	clock-names = "pixel";
> +	clocks = <&si5341 0 7>; /* Output 7 */
> +
> +	/* Set output 7 to use syntesizer 3 as its parent */
> +	assigned-clocks = <&si5341 0 7>, <&si5341 1 3>;
> +	assigned-clock-parents = <&si5341 1 3>;
> +	/* Set output 7 to 148.5 MHz using a synth frequency of 594 MHz */
> +	assigned-clock-rates = <148500000>, <594000000>;
> +};
> +
> +some-audio-node {
> +	clock-names = "i2s-clk";
> +	clocks = <&si5341 0 0>;
> +	/*
> +	 * since output 0 is a synth-master, the synth will be automatically set
> +	 * to an appropriate frequency when the audio driver requests another
> +	 * frequency. We give control over synth 2 to this output here.
> +	 */
> +	assigned-clocks = <&si5341 0 0>;
> +	assigned-clock-parents = <&si5341 1 2>;
> +};
> -- 
> 2.17.1
> 

[PATCH v3] dt-bindings: clock: Add silabs,si5341

[Mike Looijmans]

Adds the devicetree bindings for the Si5341 and Si5340 chips from
Silicon Labs. These are multiple-input multiple-output clock
synthesizers.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---
v3: Remove synthesizers child nodes
    Fix typo
v2: Add data sheet reference.
    Restructured to enable use of "assigned-clock*" properties to set
    up both outputs and internal synthesizers.
    Nicer indentation.
    Updated subject line and body of commit message.

 .../bindings/clock/silabs,si5341.txt          | 162 ++++++++++++++++++
 1 file changed, 162 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..a70c333e4cd4
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/silabs,si5341.txt
@@ -0,0 +1,162 @@
+Binding for Silicon Labs Si5341 and Si5340 programmable i2c clock generator.
+
+Reference
+[1] Si5341 Data Sheet
+    https://www.silabs.com/documents/public/data-sheets/Si5341-40-D-DataSheet.pdf
+[2] Si5341 Reference Manual
+    https://www.silabs.com/documents/public/reference-manuals/Si5341-40-D-RM.pdf
+
+The Si5341 and Si5340 are programmable i2c clock generators with up to 10 output
+clocks. The chip contains a PLL that sources 5 (or 4) multisynth clocks, which
+in turn can be directed to any of the 10 (or 4) outputs through a divider.
+The internal structure of the clock generators can be found in [2].
+
+The driver can be used in "as is" mode, reading the current settings from the
+chip at boot, in case you have a (pre-)programmed device. If the PLL is not
+configured when the driver probes, it assumes the driver must fully initialize
+it.
+
+The device type, speed grade and revision are determined runtime by probing.
+
+The driver currently only supports XTAL input mode, and does not support any
+fancy input configurations. They can still be programmed into the chip and
+the driver will leave them "as is".
+
+==I2C device node==
+
+Required properties:
+- compatible: shall be one of the following:
+	"silabs,si5340" - Si5340 A/B/C/D
+	"silabs,si5341" - Si5341 A/B/C/D
+- reg: i2c device address, usually 0x74
+- #clock-cells: from common clock binding; shall be set to 2.
+	The first value is "0" for outputs, "1" for synthesizers.
+	The second value is the output or synthesizer index.
+- clocks: from common clock binding; list of parent clock  handles,
+	corresponding to inputs. Use a fixed clock for the "xtal" input.
+	At least one must be present.
+- clock-names: One of: "xtal", "in0", "in1", "in2"
+- vdd-supply: Regulator node for VDD
+
+Optional properties:
+- vdda-supply: Regulator node for VDDA
+- vdds-supply: Regulator node for VDDS
+- 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.
+- 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.
+- interrupts: Interrupt for INTRb pin.
+- #address-cells: shall be set to 1.
+- #size-cells: shall be set to 0.
+
+
+== Child nodes: Outputs ==
+
+The child nodes list the output clocks.
+
+Each of the clock outputs can be overwritten individually by using a child 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:
+- vdd-supply: Regulator node for VDD for this output. The driver selects default
+	values for common-mode and amplitude based on the voltage.
+- silabs,format: Output format, one of:
+	1 = differential (defaults to LVDS levels)
+	2 = low-power (defaults to HCSL levels)
+	4 = LVCMOS
+- silabs,common-mode: Manually override output common mode, see [2] for values
+- silabs,amplitude: Manually override output amplitude, see [2] for values
+- silabs,synth-master: boolean. If present, this output is allowed to change the
+	multisynth frequency dynamically.
+- silabs,silabs,disable-high: boolean. If set, the clock output is driven HIGH
+	when disabled, otherwise it's driven LOW.
+
+==Example==
+
+/* 48MHz reference crystal */
+ref48: ref48M {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <48000000>;
+};
+
+i2c-master-node {
+	/* Programmable clock (for logic) */
+	si5341: clock-generator@74 {
+		reg = <0x74>;
+		compatible = "silabs,si5341";
+		#clock-cells = <2>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&ref48>;
+		clock-names = "xtal";
+
+		silabs,pll-m-num = <14000>; /* PLL at 14.0 GHz */
+		silabs,pll-m-den = <48>;
+		silabs,reprogram; /* Chips are not programmed, always reset */
+
+		out@0 {
+			reg = <0>;
+			silabs,format = <1>; /* LVDS 3v3 */
+			silabs,common-mode = <3>;
+			silabs,amplitude = <3>;
+			silabs,synth-master;
+		};
+
+		/*
+		 * Output 6 configuration:
+		 *  LVDS 1v8
+		 */
+		out@6 {
+			reg = <6>;
+			silabs,format = <1>; /* LVDS 1v8 */
+			silabs,common-mode = <13>;
+			silabs,amplitude = <3>;
+		};
+
+		/*
+		 * Output 8 configuration:
+		 *  HCSL 3v3
+		 */
+		out@8 {
+			reg = <8>;
+			silabs,format = <2>;
+			silabs,common-mode = <11>;
+			silabs,amplitude = <3>;
+		};
+	};
+};
+
+some-video-node {
+	/* Standard clock bindings */
+	clock-names = "pixel";
+	clocks = <&si5341 0 7>; /* Output 7 */
+
+	/* Set output 7 to use syntesizer 3 as its parent */
+	assigned-clocks = <&si5341 0 7>, <&si5341 1 3>;
+	assigned-clock-parents = <&si5341 1 3>;
+	/* Set output 7 to 148.5 MHz using a synth frequency of 594 MHz */
+	assigned-clock-rates = <148500000>, <594000000>;
+};
+
+some-audio-node {
+	clock-names = "i2s-clk";
+	clocks = <&si5341 0 0>;
+	/*
+	 * since output 0 is a synth-master, the synth will be automatically set
+	 * to an appropriate frequency when the audio driver requests another
+	 * frequency. We give control over synth 2 to this output here.
+	 */
+	assigned-clocks = <&si5341 0 0>;
+	assigned-clock-parents = <&si5341 1 2>;
+};
-- 
2.17.1

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

[Rob Herring]

On Fri, 17 May 2019 15:20:20 +0200, Mike Looijmans wrote:
> Adds the devicetree bindings for the Si5341 and Si5340 chips from
> Silicon Labs. These are multiple-input multiple-output clock
> synthesizers.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> v3: Remove synthesizers child nodes
>     Fix typo
> v2: Add data sheet reference.
>     Restructured to enable use of "assigned-clock*" properties to set
>     up both outputs and internal synthesizers.
>     Nicer indentation.
>     Updated subject line and body of commit message.
> 
>  .../bindings/clock/silabs,si5341.txt          | 162 ++++++++++++++++++
>  1 file changed, 162 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5341.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Stephen Boyd]

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.

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

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

[Stephen Boyd]

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.

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

[Stephen Boyd]

Quoting Mike Looijmans (2019-05-17 06:20:20)
> Adds the devicetree bindings for the Si5341 and Si5340 chips from
> Silicon Labs. These are multiple-input multiple-output clock
> synthesizers.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---

Applied to clk-next