Re: [Patch v9] driver/clk/clk-si5338: Add common clock framework driver for si5338

From: york sun
Date: Fri Sep 02 2016 - 14:32:19 EST

On 09/02/2016 07:04 AM, Rob Herring wrote:
> On Fri, Aug 26, 2016 at 02:45:49PM -0700, York Sun wrote:
>> From: York Sun <yorksun@xxxxxxxxxxxxx>
>>
>> SI5338 is a programmable clock generator. It has 4 sets of inputs,
>> PLL, multisynth and dividers to make 4 outputs. This driver splits
>> them into multiple clocks to comply with common clock framework.
>>
>> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for
>> details.
>>
>> Signed-off-by: York Sun <yorksun@xxxxxxxxxxxxx>
>> CC: Mike Turquette <mturquette@xxxxxxxxxxxx>
>> CC: Sebastian Hesselbarth <sebastian.hesselbarth@xxxxxxxxx>
>> CC: Guenter Roeck <linux@xxxxxxxxxxxx>
>> CC: Andrey Filippov <andrey@xxxxxxxxxx>
>> CC: Paul Bolle <pebolle@xxxxxxxxxx>
>
> 7 versions before you decide to start cc'ing DT list?

7? I only started to CC DT list since version 8. Didn't know I needed
to. I thought device tree binding is part of driver.

>
> [...]
>
>> diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt b/Documentation/devicetree/bindings/clock/silabs,si5338.txt
>> new file mode 100644
>> index 0000000..cc7ae8e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt
>> @@ -0,0 +1,192 @@
>> +Binding for Silicon Labs Si5338 programmable i2c clock generator.
>> +
>> +Reference
>> +[1] Si5338 Data Sheet
>> + http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf
>> +
>> +The Si5338 is a programmable i2c clock generators with up to 4 output
>> +clocks. It has 4 sets of possible input clocks
>> +
>> +IN1/IN2: differential
>> +IN3: single-ended
>> +IN4: single-ended
>> +IN5/IN6: differential
>> +
>> +Additionally, IN1/IN2 can be used as XTAL with different setting.
>> +The clock tree looks like below (without support of zero-delay)
>> +
>> +
>> + IN1/IN2 IN3 IN4 IN5/IN6
>> + | | | |
>> + ------| | | |
>> + | | | | |
>> + | \ / \ /
>> + | \ / \ /
>> + | \ / \ /
>> + XTAL REFCLK FBCLK
>> + | | \ / |
>> + | | \ / |
>> + | | DIVREFCLK DIVFBCLK |
>> + | | \ / |
>> + | | \ / |
>> + | | \ / |
>> + | | PLL |
>> + | | / | | \ |
>> + | | / / \ \ |
>> + | | / / \ \ |
>> + | | / | | \ |
>> + | | | | | | |
>> + | | MS0 MS1 MS2 MS3 |
>> + | | | | | | |
>> +
>> + OUT0 OUT1 OUT2 OUT3
>> +
>> +The output clock can choose from any of the above clock as its source, with
>> +exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3
>> +can only be used for OUT3.
>> +
>> +==I2C device node==
>> +
>> +Required properties:
>> +- compatible: shall be "silabs,si5338".
>> +- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71
>> +- #clock-cells: shall be set to 1 for multiple outputs
>> +- clocks: list of parent clocks in the order of <xtal>, <in1/2>, <in3>, <in4>,
>> + <in5/6>.
>> + Note, xtal and in1/2 are mutually exclusive. Only one can be set.
>> +- clock-names: The name of the clocks in the same order.
>> +- #address-cells: shall be set to 1.
>> +- #size-cells: shall be set to 0.
>> +- silabs,pll-master: If PLL is used, pick one MS (0, 1, 2, or 3) to allow
>> + chaning PLL rate. This is arbitrary since MS0/1/2/3 share one PLL.
>> + PLL can be calculated backward to satisfy MS. This node is not
>> + required if PLL is not used, or if silabs,pll-vco is set.
>> +
>> +Optional properties if not set by platform driver:
>> +- silab,name-prefix: name prefix for si5338
>> + If multiple si5338 chips exist, use name-prefix to form unique names.
>> + If omitted, i2c bus name will be used as the prefix.
>
> Drop this please. There should not be any need for it.

Which part? The default name, or slab,name-prefix completely?

When multiple si5338 chips exist, we need to name the clocks
differently, don't we?

>
>> +- silab,ref-source: source of refclk, valid value is defined as
>> + #define SI5338_REF_SRC_CLKIN12 0
>> + #define SI5338_REF_SRC_CLKIN3 1
>> + #define SI5338_REF_SRC_XTAL 4
>> +- silab,fb-source: source of fbclk, valid value is defined as
>> + #define SI5338_FB_SRC_CLKIN4 2
>> + #define SI5338_FB_SRC_CLKIN56 3
>> + #define SI5338_FB_SRC_NOCLK 5
>> +- silabs,pll-source: source of pll, valid value is defined as
>> + #define SI5338_PFD_IN_REF_REFCLK 0
>> + #define SI5338_PFD_IN_REF_FBCLK 1
>> + #define SI5338_PFD_IN_REF_DIVREFCLK 2
>> + #define SI5338_PFD_IN_REF_DIVFBCLK 3
>> + #define SI5338_PFD_IN_REF_XOCLK 4
>> + #define SI5338_PFD_IN_REF_NOCLK 5
>> +- silabs,pll-vco: Specify VCO frequency for optimal ratios for all outputs.
>> + If specified, silabs,pll-master is ignored.
>> +
>> +==Child nodes==
>> +
>> +Each of the clock outputs can be configured individually by
>> +using a child node to the I2C device node. If a child node for a clock
>> +output is not set, platform driver has to set up.
>> +
>> +Required child node properties:
>> +- name: name for the child node
>> + It has to be unique. The name prefix is ignored.
>> + If using platform data and the name is not specificed,
>> + clkout0/1/2/3 will be used with name prefix.
>> +- reg: number of clock output.
>> +
>> +Optional child node properties:
>> +- silabs,drive-config: the configuration of output driver
>> + The valid value list is long. Please refer to soruce code.
>
> May be long, but needs to be documented here. I don't follow why you are
> using strings for the values here.

Using string so it is readable. The acceptable values are

3V3_CMOS_A+
3V3_CMOS_A-
3V3_CMOS_B+
3V3_CMOS_B-
3V3_CMOS_A+B+
3V3_CMOS_A-B+
3V3_CMOS_A+B-
3V3_CMOS_A-B-

2V5_CMOS_A+
2V5_CMOS_A-
2V5_CMOS_B+
2V5_CMOS_B-
2V5_CMOS_A+B+
2V5_CMOS_A-B+
2V5_CMOS_A+B-
2V5_CMOS_A-B-

1V8_CMOS_A+
1V8_CMOS_A-
1V8_CMOS_B+
1V8_CMOS_B-
1V8_CMOS_A+B+
1V8_CMOS_A-B+
1V8_CMOS_A+B-
1V8_CMOS_A-B-

1V5_HSTL_A+
1V5_HSTL_A-
1V5_HSTL_B+
1V5_HSTL_B-
1V5_HSTL_A+B+
1V5_HSTL_A-B+
1V5_HSTL_A+B-
1V5_HSTL_A-B-

3V3_SSTL_A+
3V3_SSTL_A-
3V3_SSTL_B+
3V3_SSTL_B-
3V3_SSTL_A+B+
3V3_SSTL_A-B+
3V3_SSTL_A+B-
3V3_SSTL_A-B-

2V5_SSTL_A+
2V5_SSTL_A-
2V5_SSTL_B+
2V5_SSTL_B-
2V5_SSTL_A+B+
2V5_SSTL_A-B+
2V5_SSTL_A+B-
2V5_SSTL_A-B-

1V8_SSTL_A+
1V8_SSTL_A-
1V8_SSTL_B+
1V8_SSTL_B-
1V8_SSTL_A+B+
1V8_SSTL_A-B+
1V8_SSTL_A+B-
1V8_SSTL_A-B-

3V3_LVPECL
2V5_LVPECL
3V3_LVDS
2V5_LVDS
1V8_LVDS

Will add here in next version.

>
>> +- silabs,clock-source: source clock of the output divider
>> + #define SI5338_OUT_MUX_FBCLK 0
>> + #define SI5338_OUT_MUX_REFCLK 1
>> + #deinfe SI5338_OUT_MUX_DIVFBCLK 2
>> + #deinfe SI5338_OUT_MUX_DIVREFCLK 3
>> + #deinfe SI5338_OUT_MUX_XOCLK 4
>> + #deinfe SI5338_OUT_MUX_MS0 5
>> + #deinfe SI5338_OUT_MUX_MSN 6 /* MS0/1/2/3 */
>> + #deinfe SI5338_OUT_MUX_NOCLK 7
>> +- silabs,disable-state : clock output disable state, shall be
>> + #define SI5338_OUT_DIS_HIZ 0
>> + #define SI5338_OUT_DIS_LOW 1
>> + #define SI5338_OUT_DIS_HI 2
>> + #define SI5338_OUT_DIS_ALWAYS_ON 3
>> +- enabled: the output is enabled by default
>> + The existence of this node enables the output when the driver is loaded
>> + otherwise the clock is only enabled when used
>
> Drop this. Either platform code should claim this and enable the clock
> or use the critical clocks binding.

The idea is to specify if the clock(s) should be enabled upon loading
the driver. I am not familiar with critical clocks binding. Can you
point me to its document/code? Not seeing it in kernel tree.

>
>> +
>> +==Example==
>> +
>> +/* 25MHz reference crystal */
>> +ref25: ref25M {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <25000000>;
>> +};
>> +clkin56: ref100M {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <100000000>;
>> +};
>> +i2c-master-node {
>> + si5338: clock-generator@70 {
>> + compatible = "silabs,si5338";
>> + reg = <0x70>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + #clock-cells = <1>;
>> +
>> + /* connect xtal to 25MHz, in5/in6 to 100MHz */
>> + clocks = <&ref25>, <0>, <0>, <0>, <&clkin56>;
>> + clock-names = "xtal", "in12", "in3", "in4", "in56";
>> +
>> + /* connect xtal as source of refclk */
>> + silab,ref-source = <SI5338_REF_SRC_XTAL>;
>> +
>> + /* connect in5/in6 as source of fbclk */
>> + silab,fb-source = <SI5338_FB_SRC_CLKIN56>;
>> +
>> + /* connect divrefclk as source of pll */
>> + silab,pll-source = <SI5338_PFD_IN_REF_DIVREFCLK>;
>> +
>> + /* Choose one MS for pll master */
>> + silabs,pll-master = <0>;
>> +
>> + /* Specify pll-vco frequency. pll-master is ignored. */
>> + silabs,pll-vco = <2450000000>;
>> +
>> + /* output */
>> + clkout0 {
>
> reg property means you need a unit address.

The reg has the address of each output, i.e. 0, 1, 2, 3. What's wrong here?

>
>> + reg = <0>;
>> + silabs,drive-config = "1V8_LVDS";
>> + silabs,clock-source = <SI5338_OUT_MUX_MS0>;
>> + silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
>> + clock-frequency = <125000000>;
>> + };
>> + clkout1 {
>> + reg = <1>;
>> + silabs,drive-config = "1V8_LVDS";
>> + silabs,clock-source = <SI5338_OUT_MUX_MSN>;
>> + silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
>> + clock-frequency = <125000000>;
>> + };
>> + clkout2 {
>> + reg = <2>;
>> + silabs,drive-config = "1V8_LVDS";
>> + silabs,clock-source = <SI5338_OUT_MUX_MSN>;
>> + silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
>> + clock-frequency = <125000000>;
>> + };
>> + clkout3 {
>> + reg = <3>;
>> + silabs,drive-config = "1V8_LVDS";
>> + silabs,clock-source = <SI5338_OUT_MUX_MSN>;
>> + silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
>> + clock-frequency = <125000000>;
>> + };
>> +
>> + };
>> +};

I see no more comment below. Are you OK with the rest of the patch?

York