[2/8] clk: berlin: add clock binding docs for Marvell Berlin2 SoCs

Commit Message

Sebastian Hesselbarth May 11, 2014, 8:24 p.m. UTC

This adds mandatory device tree binding documentation for the clock related
IP found on Marvell Berlin2 (BG2, BG2CD, and BG2Q) SoCs.

Signed-off-by: Sebastian Hesselbarth <[e-mail]>
---
Cc: Mike Turquette <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Pawel Moll <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: Ian Campbell <[e-mail]>
Cc: Kumar Gala <[e-mail]>
Cc: Randy Dunlap <[e-mail]>
Cc: Alexandre Belloni <[e-mail]>
Cc: Antoine Tenart <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 .../devicetree/bindings/clock/berlin2-clock.txt    | 169 +++++++++++++++++++++
 1 file changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/berlin2-clock.txt

Comments

Sebastian Hesselbarth May 13, 2014, 8:38 a.m. UTC | #1

On 05/11/2014 10:24 PM, Sebastian Hesselbarth wrote:
> This adds mandatory device tree binding documentation for the clock related
> IP found on Marvell Berlin2 (BG2, BG2CD, and BG2Q) SoCs.
>
> Signed-off-by: Sebastian Hesselbarth <[e-mail]>
> ---
> Cc: Mike Turquette <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Pawel Moll <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Ian Campbell <[e-mail]>
> Cc: Kumar Gala <[e-mail]>
> Cc: Randy Dunlap <[e-mail]>
> Cc: Alexandre Belloni <[e-mail]>
> Cc: Antoine Tenart <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>   .../devicetree/bindings/clock/berlin2-clock.txt    | 169 +++++++++++++++++++++
>   1 file changed, 169 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/clock/berlin2-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/berlin2-clock.txt b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
> new file mode 100644
> index 000000000000..3da87a488402
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
> @@ -0,0 +1,169 @@
> +* Marvell Berlin2 clock bindings
> +
> +Marvell Berlin2 (BG2, BG2CD, BG2Q) share the same IP for PLLs and clocks,
> +with some minor differences in features and register layout. The below
> +describes the individual clock related IP:
> +
> +* Audio/Video PLL
> +
> +The Audio/Video PLL (AVPLL) is a dual-VCO PLL with 8 channels each. Each
> +of the VCOs can sythesize a single VCO frequency based on a single input
> +reference clock. Each of the 8 channels then, can derive an output clock
> +from that VCO frequency by various dividers/multipliers.
> +
> +Required properties:
> +- compatible: shall be "marvell,berlin2-avpll"
> +- reg: address and length of the corresponding AVPLL registers
> +- #clock-cells: shall be set to 2
> +- clocks: single clock specifier referencing the AVPLL input clock
> +
> +To ease match-up with the desired AVPLL output clock, clock specifiers
> +referencing AVPLL clocks shall contain two cells. The first refers to
> +the VCO (0=AVPLL_A, 1=AVPLL_B) while the second refers to the corresponding
> +channel starting with 1. For example, to reference AVPLL_B3 the clock
> +specifier shall be: <&avpll 1 3>.
> +
> +Example:
> +
> +avpll: pll@ea0040 {
> +	compatible = "marvell,berlin2-avpll";
> +	#clock-cells = <2>;
> +	reg = <0xea0050 0x100>;
> +	clocks = <&refclk>;
> +};
> +
> +* Simple PLLs
> +
> +Simple PLLs are memory mapped PLLs that can sythesize a single output clock
> +based on a single input reference clock.
> +
> +Required properties:
> +- compatible: shall be one of the following:
> +	"marvell,berlin2-pll" for Berlin BG2/BG2CD PLLs
> +	"marvell,berlin2q-pll" for Berlin BG2Q PLLs
> +- reg: address and length of the corresponding PLL registers
> +- #clock-cells: shall be set to 0
> +- clocks: single clock specifier referencing the PLL input clock
> +
> +Example:
> +
> +cpupll: pll@ea003c {
> +	compatible = "marvell,berlin2-pll";
> +	#clock-cells = <0>;
> +	reg = <0xea003c 0x14>;
> +	clocks = <&refclk>;
> +};
> +
> +* Single-register clock dividers
> +
> +Single-register clock dividers are complex divider cells, allowing
> +to divide a reference clock with a set of fixed dividers. Also they
> +comprise and input clock mux with bypass and an ouput clock gate.
> +
> +Required properties:
> +- compatible: shall be "marvell,berlin2-clk-div"
> +- reg: address and length of the corresponding DIV registers
> +- #clock-cells: shall be set to 0
> +- clocks: clock specifiers referencing the DIV input clocks
> +- clock-names: array of strings describing the clock specifiers above.
> +    Allowed clock-names are "mux_bypass" for the clock mux bypass selection
> +    and "muxN" (N=0..7) for each of the 8 possible clock mux inputs.
> +
> +Example:
> +
> +gfx3dcore_clk: clock@ea022c {
> +	compatible = "marvell,berlin2-clk-div";

Actually using the clock driver for SDHCI IP revealed some issues
already. Above compatible should have been "marvell,berlin2-div".
I'll fix it up for v2.

Also, the commit msg should start with 'dt-binding: clk:' I guess.

Sebastian

> +	#clock-cells = <0>;
> +	reg = <0xea0022c 0x4>;
> +	clocks = <&syspll>,
> +		<&avpll AVPLL_B 4>, <&avpll AVPLL_B 5>,
> +		<&avpll AVPLL_B 6>, <&avpll AVPLL_B 7>;
> +	clock-names = "mux_bypass",
> +		"mux0", "mux1", "mux2", "mux3";
> +};
> +
> +* SoC-specific core clocks
> +
> +In addition to the above, there is a register set dealing with SoC
> +specific clock dividers, muxes, and gates. There is also the complex
> +divider cell used above, but instead of independent registers, they
> +share a common set of registers. The core clocks are represented by
> +a single DT node providing access to the remaining clocks.
> +
> +Required properties:
> +- compatible: shall be one of
> +	"marvell,berlin2-core-clocks" for BG2/BG2CD SoCs
> +	"marvell,berlin2q-core-clocks" for BG2Q SoCs
> +- reg: address and length of the corresponding clock registers
> +- #clock-cells: shall be set to 1
> +- clocks: clock specifiers referencing the core clock input clocks
> +- clock-names: array of strings describing the clock specifiers above.
> +    Allowed clock-names for the reference clocks are
> +      "refclk", "syspll", "mempll", "cpupll"
> +    also Audio/Video PLL clocks shall be named with
> +      "avpll_VN" (V=0...1 for AVPLL_A and AVPLL_B, N=1..8 for the
> +      corresponding reference input from AVPLL).
> +
> +Optional properties for BG2/BG2CD SoCs:
> +- clocks/clock-names: in addition to the allowed clock names above,
> +    there is an external video clock input that shall be named "video_ext0".
> +
> +Clocks provided by core clocks shall be referenced by a clock specifier
> +indexing one of the provided clocks. A SoC-specific list of available clocks
> +is below the example.
> +
> +Example:
> +coreclk: clock@ea0150 {
> +	compatible = "marvell,berlin2-core-clocks";
> +	#clock-cells = <1>;
> +	reg = <0xea0150 0x1c>;
> +	clocks = <&refclk>, <&syspll>, <&mempll>, <&cpupll>,
> +		<&avpll 0 1>, <&avpll 0 2>,
> +		<&avpll 0 3>, <&avpll 0 4>,
> +		<&avpll 0 5>, <&avpll 0 6>,
> +		<&avpll 0 7>, <&avpll 0 8>,
> +		<&avpll 1 1>, <&avpll 1 2>,
> +		<&avpll 1 3>, <&avpll 1 4>,
> +		<&avpll 1 5>, <&avpll 1 6>,
> +		<&avpll 1 7>, <&avpll 1 8>,
> +		<&externalvideoclk>;
> +	clock-names = "refclk", "syspll", "mempll", "cpupll",
> +		"avpll_a1", "avpll_a2", "avpll_a3", "avpll_a4",
> +		"avpll_a5", "avpll_a6", "avpll_a7", "avpll_a8",
> +		"avpll_b1", "avpll_b2", "avpll_b3", "avpll_b4",
> +		"avpll_b5", "avpll_b6", "avpll_b7", "avpll_b8",
> +		"video_ext0";
> +};
> +
> +* BG2/BG2CD core clock indicies:
> +0  - SYS
> +1  - CPU
> +2  - DRMFIGO
> +3  - CFG
> +4  - GFX
> +5  - ZSP
> +6  - PERIF
> +7  - PCUBE
> +8  - VSCOPE
> +9  - NFC_ECC
> +10 - VPP
> +11 - APP
> +12 - AUDIO0
> +23 - AUDIO2
> +14 - AUDIO3
> +15 - AUDIO1
> +16 - GETH0
> +17 - GETH1
> +18 - SATA
> +19 - AHBAPB
> +20 - USB0
> +21 - USB1
> +22 - PBRIDGE
> +23 - SDIO0
> +24 - SDIO1
> +25 - NFC
> +26 - SMEMC
> +27 - AUDIOHD
> +28 - VIDEO0
> +29 - VIDEO1
> +30 - VIDEO2
>

Alexandre Belloni May 13, 2014, 2:47 p.m. UTC | #2

Hi,

Not much to say,

On 11/05/2014 at 22:24:35 +0200, Sebastian Hesselbarth wrote :
> +* Single-register clock dividers
> +
> +Single-register clock dividers are complex divider cells, allowing
> +to divide a reference clock with a set of fixed dividers. Also they
> +comprise and input clock mux with bypass and an ouput clock gate.
typo here-----^

> +
> +Required properties:
> +- compatible: shall be "marvell,berlin2-clk-div"
> +- reg: address and length of the corresponding DIV registers
> +- #clock-cells: shall be set to 0
> +- clocks: clock specifiers referencing the DIV input clocks
> +- clock-names: array of strings describing the clock specifiers above.
> +    Allowed clock-names are "mux_bypass" for the clock mux bypass selection
> +    and "muxN" (N=0..7) for each of the 8 possible clock mux inputs.
> +

Mike Turquette May 14, 2014, 10:32 p.m. UTC | #3

Quoting Sebastian Hesselbarth (2014-05-11 13:24:35)
> This adds mandatory device tree binding documentation for the clock related
> IP found on Marvell Berlin2 (BG2, BG2CD, and BG2Q) SoCs.
> 
> Signed-off-by: Sebastian Hesselbarth <[e-mail]>
> ---
> Cc: Mike Turquette <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Pawel Moll <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Ian Campbell <[e-mail]>
> Cc: Kumar Gala <[e-mail]>
> Cc: Randy Dunlap <[e-mail]>
> Cc: Alexandre Belloni <[e-mail]>
> Cc: Antoine Tenart <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  .../devicetree/bindings/clock/berlin2-clock.txt    | 169 +++++++++++++++++++++
>  1 file changed, 169 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/berlin2-clock.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/berlin2-clock.txt b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
> new file mode 100644
> index 000000000000..3da87a488402
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
> @@ -0,0 +1,169 @@
> +* Marvell Berlin2 clock bindings
> +
> +Marvell Berlin2 (BG2, BG2CD, BG2Q) share the same IP for PLLs and clocks,
> +with some minor differences in features and register layout. The below
> +describes the individual clock related IP:
> +
> +* Audio/Video PLL
> +
> +The Audio/Video PLL (AVPLL) is a dual-VCO PLL with 8 channels each. Each
> +of the VCOs can sythesize a single VCO frequency based on a single input
> +reference clock. Each of the 8 channels then, can derive an output clock
> +from that VCO frequency by various dividers/multipliers.
> +
> +Required properties:
> +- compatible: shall be "marvell,berlin2-avpll"
> +- reg: address and length of the corresponding AVPLL registers
> +- #clock-cells: shall be set to 2
> +- clocks: single clock specifier referencing the AVPLL input clock
> +
> +To ease match-up with the desired AVPLL output clock, clock specifiers
> +referencing AVPLL clocks shall contain two cells. The first refers to
> +the VCO (0=AVPLL_A, 1=AVPLL_B) while the second refers to the corresponding
> +channel starting with 1. For example, to reference AVPLL_B3 the clock
> +specifier shall be: <&avpll 1 3>.
> +
> +Example:
> +
> +avpll: pll@ea0040 {
> +       compatible = "marvell,berlin2-avpll";
> +       #clock-cells = <2>;
> +       reg = <0xea0050 0x100>;
> +       clocks = <&refclk>;
> +};

Hi Sebastian,

Thanks for submitting the series. It looks good. I do have some comments
about the DT bindings though. I'm encouraging new bindings (and
especially new platforms or existing platforms that are only now
converting over to CCF) to not put their per-clock data into DTS. This
has scalability problems, is unpopular with the DT crowd and sometimes
makes it hard to do things like set CLK_SET_RATE_PARENT flags for
individual clocks.

The following is a copy/paste from an email I sent earlier today[1]. Of
course per-clock data makes great sense if you have an off-SoC clock
such as a fixed-rate oscillator (e.g. the fixed-clock binding). Let me
know what you think:

I assume the rest of your clocks are part of a clock generator IP block
inside of your chip. Have you looked at the QCOM binding? It is my
favorite binding these days. Here are some highlights:

See Documentation/devicetree/bindings/clock/qcom,gcc.txt.



From arch/arm/boot/dts/qcom-msm8974.dtsi:

gcc: clock-controller@fc400000 {
        compatible = "qcom,gcc-msm8974";
        #clock-cells = <1>;
        #reset-cells = <1>;
        reg = <0xfc400000 0x4000>;
};

...

serial@f991e000 {
        compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
        reg = <0xf991e000 0x1000>;
        interrupts = <0 108 0x0>;
        clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>, <&gcc GCC_BLSP1_AHB_CLK>;
        clock-names = "core", "iface";
};



From drivers/clk/qcom/gcc-msm8974.c:

static struct clk_branch gcc_blsp1_uart2_apps_clk = {
        .halt_reg = 0x0704,
        .clkr = {
                .enable_reg = 0x0704,
                .enable_mask = BIT(0),
                .hw.init = &(struct clk_init_data){
                        .name = "gcc_blsp1_uart2_apps_clk",
                        .parent_names = (const char *[]){
                                "blsp1_uart2_apps_clk_src",
                        },
                        .num_parents = 1,
                        .flags = CLK_SET_RATE_PARENT,
                        .ops = &clk_branch2_ops,
                },
        },
};

Using this type of binding you only need to declare your clock generator
IP node in dts, and then define a mapping in the DT include chroot. Then
you can define your per-clock data inside of your clock driver instead
of putting all of the details inside of DT.

If you have a strong reason to do it the way that you originally posted
then let me know.

Regards,
Mike

[1] https://lkml.org/lkml/2014/5/14/598

Sebastian Hesselbarth May 14, 2014, 11:17 p.m. UTC | #4

On 05/15/2014 12:32 AM, Mike Turquette wrote:
> Quoting Sebastian Hesselbarth (2014-05-11 13:24:35)
>> This adds mandatory device tree binding documentation for the clock related
>> IP found on Marvell Berlin2 (BG2, BG2CD, and BG2Q) SoCs.
>>
>> Signed-off-by: Sebastian Hesselbarth <[e-mail]>
>> ---
>> Cc: Mike Turquette <[e-mail]>
>> Cc: Rob Herring <[e-mail]>
>> Cc: Pawel Moll <[e-mail]>
>> Cc: Mark Rutland <[e-mail]>
>> Cc: Ian Campbell <[e-mail]>
>> Cc: Kumar Gala <[e-mail]>
>> Cc: Randy Dunlap <[e-mail]>
>> Cc: Alexandre Belloni <[e-mail]>
>> Cc: Antoine Tenart <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>  .../devicetree/bindings/clock/berlin2-clock.txt    | 169 +++++++++++++++++++++
>>  1 file changed, 169 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/berlin2-clock.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/berlin2-clock.txt b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
>> new file mode 100644
>> index 000000000000..3da87a488402
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
>> @@ -0,0 +1,169 @@
>> +* Marvell Berlin2 clock bindings
>> +
>> +Marvell Berlin2 (BG2, BG2CD, BG2Q) share the same IP for PLLs and clocks,
>> +with some minor differences in features and register layout. The below
>> +describes the individual clock related IP:
>> +
>> +* Audio/Video PLL
>> +
>> +The Audio/Video PLL (AVPLL) is a dual-VCO PLL with 8 channels each. Each
>> +of the VCOs can sythesize a single VCO frequency based on a single input
>> +reference clock. Each of the 8 channels then, can derive an output clock
>> +from that VCO frequency by various dividers/multipliers.
>> +
>> +Required properties:
>> +- compatible: shall be "marvell,berlin2-avpll"
>> +- reg: address and length of the corresponding AVPLL registers
>> +- #clock-cells: shall be set to 2
>> +- clocks: single clock specifier referencing the AVPLL input clock
>> +
>> +To ease match-up with the desired AVPLL output clock, clock specifiers
>> +referencing AVPLL clocks shall contain two cells. The first refers to
>> +the VCO (0=AVPLL_A, 1=AVPLL_B) while the second refers to the corresponding
>> +channel starting with 1. For example, to reference AVPLL_B3 the clock
>> +specifier shall be: <&avpll 1 3>.
>> +
>> +Example:
>> +
>> +avpll: pll@ea0040 {
>> +       compatible = "marvell,berlin2-avpll";
>> +       #clock-cells = <2>;
>> +       reg = <0xea0050 0x100>;
>> +       clocks = <&refclk>;
>> +};
> 
> Thanks for submitting the series. It looks good. I do have some comments
> about the DT bindings though. I'm encouraging new bindings (and
> especially new platforms or existing platforms that are only now
> converting over to CCF) to not put their per-clock data into DTS. This
> has scalability problems, is unpopular with the DT crowd and sometimes
> makes it hard to do things like set CLK_SET_RATE_PARENT flags for
> individual clocks.

Ok, so you are proposing the have a single node for all the SoCs
internal plls and clocks. The individual SoCs will have to deal
with the differences in a single driver, right?

> The following is a copy/paste from an email I sent earlier today[1]. Of
> course per-clock data makes great sense if you have an off-SoC clock
> such as a fixed-rate oscillator (e.g. the fixed-clock binding). Let me
> know what you think:
[...]
> Using this type of binding you only need to declare your clock generator
> IP node in dts, and then define a mapping in the DT include chroot. Then
> you can define your per-clock data inside of your clock driver instead
> of putting all of the details inside of DT.
> 
> If you have a strong reason to do it the way that you originally posted
> then let me know.

Actually, the intermediate patch set sent before this one had a single
DT clock node. The most important draw-back of a single clock node
is that Berlin's global registers are more like a register dumpster.
Vital other registers, e.g. reset, are intermixed with clock registers.

Given the lack of public datasheets (I look everything up in some
auto-generated BSP includes), I like the current approach because it
helps to get in at least some structure to the register mess ;)

Considering the postponed of_clk_create_name() helper, that would allow
us to remove at least the names from DT again. Another option would be
a syscon node for the registers, that clk, reset, pinctrl drivers can
access. But IIRC early syscon support isn't settled, yet?

So, my current idea is:
- take this as is, stabilize berlin branches for v3.16
- review of_clk_create_name() and of_clk_get_parent_name() to allow
  to remove clock-output-names properties from Berlin (and other) dtsis
- maybe switch to early syscon if it is available in v3.16

I know this would likely break DT ABI policy, but hey who else boots
mainline Linux on his Chromecast currently except me :P

Is that okay with you (and DT folks)?

Sebastian

Mike Turquette May 15, 2014, 4:41 a.m. UTC | #5

Quoting Sebastian Hesselbarth (2014-05-14 16:17:52)
> On 05/15/2014 12:32 AM, Mike Turquette wrote:
> > Quoting Sebastian Hesselbarth (2014-05-11 13:24:35)
> >> +avpll: pll@ea0040 {
> >> +       compatible = "marvell,berlin2-avpll";
> >> +       #clock-cells = <2>;
> >> +       reg = <0xea0050 0x100>;
> >> +       clocks = <&refclk>;
> >> +};
> > 
> > Thanks for submitting the series. It looks good. I do have some comments
> > about the DT bindings though. I'm encouraging new bindings (and
> > especially new platforms or existing platforms that are only now
> > converting over to CCF) to not put their per-clock data into DTS. This
> > has scalability problems, is unpopular with the DT crowd and sometimes
> > makes it hard to do things like set CLK_SET_RATE_PARENT flags for
> > individual clocks.
> 
> Ok, so you are proposing the have a single node for all the SoCs
> internal plls and clocks. The individual SoCs will have to deal
> with the differences in a single driver, right?

To be precise, I'm talking about modeling an IP block as a single node.
So if you have one clock generator IP block then you have one node. If
you have more than one clock generator block then you have more than one
node. Re-using the qcom example there are compatible strings for two
different clock generator blocks named gcc and mmcc, respectively. So
two DT nodes in the case for msm platforms that have one gcc instance
and one rcg instance.

Additionally other IP blocks may have internal clocks that can be
modeled as part of that node. OMAP's Display SubSystem (DSS) and Image
Signal Processor (ISP) blocks all have internal clocks that are modeled
through the clock framework. (There are no DT bindings for that stuff,
but the concept still applies)

> 
> > The following is a copy/paste from an email I sent earlier today[1]. Of
> > course per-clock data makes great sense if you have an off-SoC clock
> > such as a fixed-rate oscillator (e.g. the fixed-clock binding). Let me
> > know what you think:
> [...]
> > Using this type of binding you only need to declare your clock generator
> > IP node in dts, and then define a mapping in the DT include chroot. Then
> > you can define your per-clock data inside of your clock driver instead
> > of putting all of the details inside of DT.
> > 
> > If you have a strong reason to do it the way that you originally posted
> > then let me know.
> 
> Actually, the intermediate patch set sent before this one had a single
> DT clock node. The most important draw-back of a single clock node
> is that Berlin's global registers are more like a register dumpster.
> Vital other registers, e.g. reset, are intermixed with clock registers.

Yeah, this is pretty common. The compatible string should reflect the IP
block as a whole, not just the clocks part of it. Lots of vendors have
PRCMs or PRCMUs or CARs or whatever.

Check out the recent series to have the reset bits and regulator support
added to the qcom binding[1]. (I'm using qcom quite a bit in my examples
but they are not the first to add reset control to their clock driver. I
think Tegra did it first...)

> 
> Given the lack of public datasheets (I look everything up in some
> auto-generated BSP includes), I like the current approach because it
> helps to get in at least some structure to the register mess ;)
> 
> Considering the postponed of_clk_create_name() helper, that would allow
> us to remove at least the names from DT again. Another option would be
> a syscon node for the registers, that clk, reset, pinctrl drivers can
> access. But IIRC early syscon support isn't settled, yet?

Yeah, I'm not sure of the state of syscon. And modeling this stuff in
the clock driver isn't the end of the world. There might be better
places than drivers/clk/* for sure... I sometimes joke that the name of
the IP block determines where the code lands. If it is Power, Reset &
Clock Manager (several platforms use this acronym) then it can end up in
drivers/clk or drivers/reset really easily. Same for Clock and Reset IP
blocks (Tegra).

> 
> So, my current idea is:
> - take this as is, stabilize berlin branches for v3.16
> - review of_clk_create_name() and of_clk_get_parent_name() to allow
>   to remove clock-output-names properties from Berlin (and other) dtsis
> - maybe switch to early syscon if it is available in v3.16
> 
> I know this would likely break DT ABI policy, but hey who else boots
> mainline Linux on his Chromecast currently except me :P

I'm not a big fan of DT stable ABI, but if you plan on changing it for
3.17 why not just do it the right way the first time? And switching to
syscon is not a hard requirement. I'm OK with you putting the reset and
regulator stuff in the clock driver if that makes the most sense for
your platform (especially if registers are shared and the same locks
need to be used, etc).

What do you think?

Regards,
Mike

[1] https://lkml.org/lkml/2014/4/4/568

> 
> Is that okay with you (and DT folks)?
> 
> Sebastian

Sebastian Hesselbarth May 15, 2014, 6:53 a.m. UTC | #6

On 05/15/2014 06:41 AM, Mike Turquette wrote:
> Quoting Sebastian Hesselbarth (2014-05-14 16:17:52)
>> On 05/15/2014 12:32 AM, Mike Turquette wrote:
>>> Quoting Sebastian Hesselbarth (2014-05-11 13:24:35)
>>>> +avpll: pll@ea0040 {
>>>> +       compatible = "marvell,berlin2-avpll";
>>>> +       #clock-cells = <2>;
>>>> +       reg = <0xea0050 0x100>;
>>>> +       clocks = <&refclk>;
>>>> +};
>>>
>>> Thanks for submitting the series. It looks good. I do have some comments
>>> about the DT bindings though. I'm encouraging new bindings (and
>>> especially new platforms or existing platforms that are only now
>>> converting over to CCF) to not put their per-clock data into DTS. This
>>> has scalability problems, is unpopular with the DT crowd and sometimes
>>> makes it hard to do things like set CLK_SET_RATE_PARENT flags for
>>> individual clocks.
>>
>> Ok, so you are proposing the have a single node for all the SoCs
>> internal plls and clocks. The individual SoCs will have to deal
>> with the differences in a single driver, right?
> 
> To be precise, I'm talking about modeling an IP block as a single node.
> So if you have one clock generator IP block then you have one node. If
> you have more than one clock generator block then you have more than one
> node. Re-using the qcom example there are compatible strings for two
> different clock generator blocks named gcc and mmcc, respectively. So
> two DT nodes in the case for msm platforms that have one gcc instance
> and one rcg instance.

Hmm, I'd argue that you'd identify an IP block by the price tag is
carries. You can buy a single PLL but given the vast amount of different
register sets for PLLs, it is hard to identify what still count for the
same IP.

> Additionally other IP blocks may have internal clocks that can be
> modeled as part of that node. OMAP's Display SubSystem (DSS) and Image
> Signal Processor (ISP) blocks all have internal clocks that are modeled
> through the clock framework. (There are no DT bindings for that stuff,
> but the concept still applies)

Agreed. If we hit any clock mux/divider/gate in any other register set,
I wouldn't think of putting it into the core clock driver but the IP
driver instead.

>>> If you have a strong reason to do it the way that you originally posted
>>> then let me know.
>>
>> Actually, the intermediate patch set sent before this one had a single
>> DT clock node. The most important draw-back of a single clock node
>> is that Berlin's global registers are more like a register dumpster.
>> Vital other registers, e.g. reset, are intermixed with clock registers.
> 
> Yeah, this is pretty common. The compatible string should reflect the IP
> block as a whole, not just the clocks part of it. Lots of vendors have
> PRCMs or PRCMUs or CARs or whatever.
> 
> Check out the recent series to have the reset bits and regulator support
> added to the qcom binding[1]. (I'm using qcom quite a bit in my examples
> but they are not the first to add reset control to their clock driver. I
> think Tegra did it first...)

Yeah, I have to think about it a while. The register block we are
talking about contains - from what I remember from the ~20k lines
include - pinctrl, padctrl, reset, clocks, secondary cpu boot related
registers.

Maybe it is time to admit that these registers will never be split into
separate blocks but should be dealt with a single node.

>> Given the lack of public datasheets (I look everything up in some
>> auto-generated BSP includes), I like the current approach because it
>> helps to get in at least some structure to the register mess ;)
>>
>> Considering the postponed of_clk_create_name() helper, that would allow
>> us to remove at least the names from DT again. Another option would be
>> a syscon node for the registers, that clk, reset, pinctrl drivers can
>> access. But IIRC early syscon support isn't settled, yet?
> 
> Yeah, I'm not sure of the state of syscon. And modeling this stuff in
> the clock driver isn't the end of the world. There might be better
> places than drivers/clk/* for sure... I sometimes joke that the name of
> the IP block determines where the code lands. If it is Power, Reset &
> Clock Manager (several platforms use this acronym) then it can end up in
> drivers/clk or drivers/reset really easily. Same for Clock and Reset IP
> blocks (Tegra).
> 
>>
>> So, my current idea is:
>> - take this as is, stabilize berlin branches for v3.16
>> - review of_clk_create_name() and of_clk_get_parent_name() to allow
>>   to remove clock-output-names properties from Berlin (and other) dtsis
>> - maybe switch to early syscon if it is available in v3.16
>>
>> I know this would likely break DT ABI policy, but hey who else boots
>> mainline Linux on his Chromecast currently except me :P
> 
> I'm not a big fan of DT stable ABI, but if you plan on changing it for
> 3.17 why not just do it the right way the first time? And switching to
> syscon is not a hard requirement. I'm OK with you putting the reset and
> regulator stuff in the clock driver if that makes the most sense for
> your platform (especially if registers are shared and the same locks
> need to be used, etc).
> 
> What do you think?

Currently, I think that a single node for the global registers with reg
property and different nodes for clock/reset and pinctrl would be best.
I think I can workaround missing early syscon with atomic_io for now and
have a syscon provided regmap later:

global: registers@ea0000 {
	compatible = "marvell,berlin2-global-registers";
	reg = <0xea0000 0x400>;
};

pinctrl: pin-controller {
	compatible = "marvell,berlin2-pinctrl";
	...
};

clocks: clocks {
	compatible = "marvell,berlin2-clocks";
	#clock-cells = <1>;
	/* or clocks and reset FWIW */
};

or on a sub-node basis:

global: registers@ea0000 {
	compatible = "marvell,berlin2-global-registers";
	reg = <0xea0000 0x400>;
	#clock-cells = <1>;
	#reset-cells = <1>;

	pinctrl: pin-controller {
		compatible = "marvell,berlin2-pinctrl";
		...
	};
};

But I haven't made up my mind, yet.

Sebastian

Alexandre Belloni May 15, 2014, 8:34 a.m. UTC | #7

On 14/05/2014 at 21:41:06 -0700, Mike Turquette wrote :
> > > The following is a copy/paste from an email I sent earlier today[1]. Of
> > > course per-clock data makes great sense if you have an off-SoC clock
> > > such as a fixed-rate oscillator (e.g. the fixed-clock binding). Let me
> > > know what you think:
> > [...]
> > > Using this type of binding you only need to declare your clock generator
> > > IP node in dts, and then define a mapping in the DT include chroot. Then
> > > you can define your per-clock data inside of your clock driver instead
> > > of putting all of the details inside of DT.
> > > 
> > > If you have a strong reason to do it the way that you originally posted
> > > then let me know.
> > 
> > Actually, the intermediate patch set sent before this one had a single
> > DT clock node. The most important draw-back of a single clock node
> > is that Berlin's global registers are more like a register dumpster.
> > Vital other registers, e.g. reset, are intermixed with clock registers.
> 
> Yeah, this is pretty common. The compatible string should reflect the IP
> block as a whole, not just the clocks part of it. Lots of vendors have
> PRCMs or PRCMUs or CARs or whatever.
> 
> Check out the recent series to have the reset bits and regulator support
> added to the qcom binding[1]. (I'm using qcom quite a bit in my examples
> but they are not the first to add reset control to their clock driver. I
> think Tegra did it first...)
> 
> > 
> > Given the lack of public datasheets (I look everything up in some
> > auto-generated BSP includes), I like the current approach because it
> > helps to get in at least some structure to the register mess ;)
> > 
> > Considering the postponed of_clk_create_name() helper, that would allow
> > us to remove at least the names from DT again. Another option would be
> > a syscon node for the registers, that clk, reset, pinctrl drivers can
> > access. But IIRC early syscon support isn't settled, yet?
> 
> Yeah, I'm not sure of the state of syscon. And modeling this stuff in
> the clock driver isn't the end of the world. There might be better
> places than drivers/clk/* for sure... I sometimes joke that the name of
> the IP block determines where the code lands. If it is Power, Reset &
> Clock Manager (several platforms use this acronym) then it can end up in
> drivers/clk or drivers/reset really easily. Same for Clock and Reset IP
> blocks (Tegra).
> 

So, this is called "global". On BG2/BG2CD, you have pinmuxing, a
register enabling nand power and switching the pll bypasses, all the pll
configuration, complex clocks, reset, product ID, nand write protect,
simple clocks, more resets, more clocks for which we don't have a driver
yet and finally pad control.

Now, on BG2Q, it is almost the same but between the cpupll registers and
the global registers, there is the whole APB bus, so timers, gpio
controllers, ...

I would believe that is the strong reason you are asking for.

It would also break the DT ABI (not that I care) almost each time we add
a new clock that we didn't know about.

Let me suggest something completely wild (to rephrase Thomas Petazzoni
and Kevin Hilman at ELC):

How about we have only a "marvell,berlin2", "marvell,berlin2cd" or
"marvell,berlin2q" compatible string and handle that in either
arch/arm/mach-berlin or driver/soc/mach-berlin and then use a bunch of
structures to add all the peripherals like clocks, reset, timer, gpio
controllers, pinmux, padconf...
Maybe we could call those structures "platform_data" :)


> > 
> > So, my current idea is:
> > - take this as is, stabilize berlin branches for v3.16
> > - review of_clk_create_name() and of_clk_get_parent_name() to allow
> >   to remove clock-output-names properties from Berlin (and other) dtsis
> > - maybe switch to early syscon if it is available in v3.16
> > 
> > I know this would likely break DT ABI policy, but hey who else boots
> > mainline Linux on his Chromecast currently except me :P
> 
> I'm not a big fan of DT stable ABI, but if you plan on changing it for
> 3.17 why not just do it the right way the first time? And switching to
> syscon is not a hard requirement. I'm OK with you putting the reset and
> regulator stuff in the clock driver if that makes the most sense for
> your platform (especially if registers are shared and the same locks
> need to be used, etc).
>

Patch

diff --git a/Documentation/devicetree/bindings/clock/berlin2-clock.txt b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
new file mode 100644
index 000000000000..3da87a488402
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/berlin2-clock.txt
@@ -0,0 +1,169 @@ 
+* Marvell Berlin2 clock bindings
+
+Marvell Berlin2 (BG2, BG2CD, BG2Q) share the same IP for PLLs and clocks,
+with some minor differences in features and register layout. The below
+describes the individual clock related IP:
+
+* Audio/Video PLL
+
+The Audio/Video PLL (AVPLL) is a dual-VCO PLL with 8 channels each. Each
+of the VCOs can sythesize a single VCO frequency based on a single input
+reference clock. Each of the 8 channels then, can derive an output clock
+from that VCO frequency by various dividers/multipliers.
+
+Required properties:
+- compatible: shall be "marvell,berlin2-avpll"
+- reg: address and length of the corresponding AVPLL registers
+- #clock-cells: shall be set to 2
+- clocks: single clock specifier referencing the AVPLL input clock
+
+To ease match-up with the desired AVPLL output clock, clock specifiers
+referencing AVPLL clocks shall contain two cells. The first refers to
+the VCO (0=AVPLL_A, 1=AVPLL_B) while the second refers to the corresponding
+channel starting with 1. For example, to reference AVPLL_B3 the clock
+specifier shall be: <&avpll 1 3>.
+
+Example:
+
+avpll: pll@ea0040 {
+	compatible = "marvell,berlin2-avpll";
+	#clock-cells = <2>;
+	reg = <0xea0050 0x100>;
+	clocks = <&refclk>;
+};
+
+* Simple PLLs
+
+Simple PLLs are memory mapped PLLs that can sythesize a single output clock
+based on a single input reference clock.
+
+Required properties:
+- compatible: shall be one of the following:
+	"marvell,berlin2-pll" for Berlin BG2/BG2CD PLLs
+	"marvell,berlin2q-pll" for Berlin BG2Q PLLs
+- reg: address and length of the corresponding PLL registers
+- #clock-cells: shall be set to 0
+- clocks: single clock specifier referencing the PLL input clock
+
+Example:
+
+cpupll: pll@ea003c {
+	compatible = "marvell,berlin2-pll";
+	#clock-cells = <0>;
+	reg = <0xea003c 0x14>;
+	clocks = <&refclk>;
+};
+
+* Single-register clock dividers
+
+Single-register clock dividers are complex divider cells, allowing
+to divide a reference clock with a set of fixed dividers. Also they
+comprise and input clock mux with bypass and an ouput clock gate.
+
+Required properties:
+- compatible: shall be "marvell,berlin2-clk-div"
+- reg: address and length of the corresponding DIV registers
+- #clock-cells: shall be set to 0
+- clocks: clock specifiers referencing the DIV input clocks
+- clock-names: array of strings describing the clock specifiers above.
+    Allowed clock-names are "mux_bypass" for the clock mux bypass selection
+    and "muxN" (N=0..7) for each of the 8 possible clock mux inputs.
+
+Example:
+
+gfx3dcore_clk: clock@ea022c {
+	compatible = "marvell,berlin2-clk-div";
+	#clock-cells = <0>;
+	reg = <0xea0022c 0x4>;
+	clocks = <&syspll>,
+		<&avpll AVPLL_B 4>, <&avpll AVPLL_B 5>,
+		<&avpll AVPLL_B 6>, <&avpll AVPLL_B 7>;
+	clock-names = "mux_bypass",
+		"mux0", "mux1", "mux2", "mux3";
+};
+
+* SoC-specific core clocks
+
+In addition to the above, there is a register set dealing with SoC
+specific clock dividers, muxes, and gates. There is also the complex
+divider cell used above, but instead of independent registers, they
+share a common set of registers. The core clocks are represented by
+a single DT node providing access to the remaining clocks.
+
+Required properties:
+- compatible: shall be one of
+	"marvell,berlin2-core-clocks" for BG2/BG2CD SoCs
+	"marvell,berlin2q-core-clocks" for BG2Q SoCs
+- reg: address and length of the corresponding clock registers
+- #clock-cells: shall be set to 1
+- clocks: clock specifiers referencing the core clock input clocks
+- clock-names: array of strings describing the clock specifiers above.
+    Allowed clock-names for the reference clocks are
+      "refclk", "syspll", "mempll", "cpupll"
+    also Audio/Video PLL clocks shall be named with
+      "avpll_VN" (V=0...1 for AVPLL_A and AVPLL_B, N=1..8 for the
+      corresponding reference input from AVPLL).
+
+Optional properties for BG2/BG2CD SoCs:
+- clocks/clock-names: in addition to the allowed clock names above,
+    there is an external video clock input that shall be named "video_ext0".
+
+Clocks provided by core clocks shall be referenced by a clock specifier
+indexing one of the provided clocks. A SoC-specific list of available clocks
+is below the example.
+
+Example:
+coreclk: clock@ea0150 {
+	compatible = "marvell,berlin2-core-clocks";
+	#clock-cells = <1>;
+	reg = <0xea0150 0x1c>;
+	clocks = <&refclk>, <&syspll>, <&mempll>, <&cpupll>,
+		<&avpll 0 1>, <&avpll 0 2>,
+		<&avpll 0 3>, <&avpll 0 4>,
+		<&avpll 0 5>, <&avpll 0 6>,
+		<&avpll 0 7>, <&avpll 0 8>,
+		<&avpll 1 1>, <&avpll 1 2>,
+		<&avpll 1 3>, <&avpll 1 4>,
+		<&avpll 1 5>, <&avpll 1 6>,
+		<&avpll 1 7>, <&avpll 1 8>,
+		<&externalvideoclk>;
+	clock-names = "refclk", "syspll", "mempll", "cpupll",
+		"avpll_a1", "avpll_a2", "avpll_a3", "avpll_a4",
+		"avpll_a5", "avpll_a6", "avpll_a7", "avpll_a8",
+		"avpll_b1", "avpll_b2", "avpll_b3", "avpll_b4",
+		"avpll_b5", "avpll_b6", "avpll_b7", "avpll_b8",
+		"video_ext0";
+};
+
+* BG2/BG2CD core clock indicies:
+0  - SYS
+1  - CPU
+2  - DRMFIGO
+3  - CFG
+4  - GFX
+5  - ZSP
+6  - PERIF
+7  - PCUBE
+8  - VSCOPE
+9  - NFC_ECC
+10 - VPP
+11 - APP
+12 - AUDIO0
+23 - AUDIO2
+14 - AUDIO3
+15 - AUDIO1
+16 - GETH0
+17 - GETH1
+18 - SATA
+19 - AHBAPB
+20 - USB0
+21 - USB1
+22 - PBRIDGE
+23 - SDIO0
+24 - SDIO1
+25 - NFC
+26 - SMEMC
+27 - AUDIOHD
+28 - VIDEO0
+29 - VIDEO1
+30 - VIDEO2