Message-ID: <20200514191318.GA10192@bogus>
Date:   Thu, 14 May 2020 14:13:18 -0500
From:   Rob Herring <robh@...nel.org>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>
Cc:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Serge Semin <fancer.lancer@...il.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Arnd Bergmann <arnd@...db.de>, linux-mips@...r.kernel.org,
        linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] dt-bindings: clk: Add Baikal-T1 CCU PLLs binding

On Thu, May 07, 2020 at 01:22:57AM +0300, Serge Semin wrote:
> Baikal-T1 Clocks Control Unit is responsible for transformation of a
> signal coming from an external oscillator into clocks of various
> frequencies to propagate them then to the corresponding clocks
> consumers (either individual IP-blocks or clock domains). In order
> to create a set of high-frequency clocks the external signal is
> firstly handled by the embedded into CCU PLLs. So the corresponding
> dts-node is just a normal clock-provider node with standard set of
> properties. Note as being part of the Baikal-T1 System Controller its
> DT node is supposed to be a child the system controller node.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
> Cc: Paul Burton <paulburton@...nel.org>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: linux-mips@...r.kernel.org
> 
> ---
> 
> Changelog v2:
> - Rearrange the SoBs.
> - Discard comments in the bindings file header.
> - Add dual GPL/BSD license.
> - Add spaces around the ASCII-graphics in the binding description.
> - Remove reference to Documentation/devicetree/bindings/clock/clock-bindings.txt
>   file.
> - Discard redundant object check against "/schemas/clock/clock.yaml#" schema.
> - Discard redundant descriptions of the "#clock-cells" property.
> - Remove "reg" property since from now the clock DT node is supposed to be
>   a child of the syscon-compatible system controller node.
> - Remove "clock-output-names" property support.
> - Replace "additionalProperties: false" with "unevaluatedProperties: false".
> - Lowercase the nodes name in the examples.
> - Use "clock-controller" node name suffix in the examples.
> - Remove unnecessary comments in the clocks dt-bindings header file.
> ---
>  .../bindings/clock/baikal,bt1-ccu-pll.yaml    | 127 ++++++++++++++++++
>  include/dt-bindings/clock/bt1-ccu.h           |  16 +++
>  2 files changed, 143 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml
>  create mode 100644 include/dt-bindings/clock/bt1-ccu.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml b/Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml
> new file mode 100644
> index 000000000000..571181758ef2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/baikal,bt1-ccu-pll.yaml
> @@ -0,0 +1,127 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/baikal,bt1-ccu-pll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Baikal-T1 Clock Control Unit PLL
> +
> +maintainers:
> +  - Serge Semin <fancer.lancer@...il.com>
> +
> +description: |
> +  Clocks Control Unit is the core of Baikal-T1 SoC System Controller
> +  responsible for the chip subsystems clocking and resetting. The CCU is
> +  connected with an external fixed rate oscillator, which signal is transformed
> +  into clocks of various frequencies and then propagated to either individual
> +  IP-blocks or to groups of blocks (clock domains). The transformation is done
> +  by means of PLLs and gateable/non-gateable dividers embedded into the CCU.
> +  It's logically divided into the next components:
> +  1) External oscillator (normally XTAL's 25 MHz crystal oscillator, but
> +     in general can provide any frequency supported by the CCU PLLs).
> +  2) PLLs clocks generators (PLLs) - described in this binding file.
> +  3) AXI-bus clock dividers (AXI).
> +  4) System devices reference clock dividers (SYS).
> +  which are connected with each other as shown on the next figure:
> +
> +          +---------------+
> +          | Baikal-T1 CCU |
> +          |   +----+------|- MIPS P5600 cores
> +          | +-|PLLs|------|- DDR controller
> +          | | +----+      |
> +  +----+  | |  |  |       |
> +  |XTAL|--|-+  |  | +---+-|
> +  +----+  | |  |  +-|AXI|-|- AXI-bus
> +          | |  |    +---+-|
> +          | |  |          |
> +          | |  +----+---+-|- APB-bus
> +          | +-------|SYS|-|- Low-speed Devices
> +          |         +---+-|- High-speed Devices
> +          +---------------+

Are you going to just duplicate all this for each sub-block? 

> +
> +  Each CCU sub-block is represented as a separate dts-node and has an
> +  individual driver to be bound with.
> +
> +  In order to create signals of wide range frequencies the external oscillator
> +  output is primarily connected to a set of CCU PLLs. There are five PLLs
> +  to create a clock for the MIPS P5600 cores, the embedded DDR controller,
> +  SATA, Ethernet and PCIe domains. The last three domains though named by the
> +  biggest system interfaces in fact include nearly all of the rest SoC
> +  peripherals. Each of the PLLs is based on True Circuits TSMC CLN28HPM core
> +  with an interface wrapper (so called safe PLL' clocks switcher) to simplify
> +  the PLL configuration procedure. The PLLs work as depicted on the next
> +  diagram:
> +
> +      +--------------------------+
> +      |                          |
> +      +-->+---+    +---+   +---+ |  +---+   0|\
> +  CLKF--->|/NF|--->|PFD|...|VCO|-+->|/OD|--->| |
> +          +---+ +->+---+   +---+ /->+---+    | |--->CLKOUT
> +  CLKOD---------C----------------+          1| |
> +       +--------C--------------------------->|/
> +       |        |                             ^
> +  Rclk-+->+---+ |                             |
> +  CLKR--->|/NR|-+                             |
> +          +---+                               |
> +  BYPASS--------------------------------------+
> +  BWADJ--->
> +
> +  where Rclk is the reference clock coming  from XTAL, NR - reference clock
> +  divider, NF - PLL clock multiplier, OD - VCO output clock divider, CLKOUT -
> +  output clock, BWADJ is the PLL bandwidth adjustment parameter. At this moment
> +  the binding supports the PLL dividers configuration in accordance with a
> +  requested rate, while bypassing and bandwidth adjustment settings can be
> +  added in future if it gets to be necessary.
> +
> +  The PLLs CLKOUT is then either directly connected with the corresponding
> +  clocks consumer (like P5600 cores or DDR controller) or passed over a CCU
> +  divider to create a signal required for the clock domain.
> +
> +  The CCU PLL dts-node uses the common clock bindings with no custom
> +  parameters. The list of exported clocks can be found in
> +  'include/dt-bindings/clock/bt1-ccu.h'. Since CCU PLL is a part of the
> +  Baikal-T1 SoC System Controller its DT node is supposed to be a child of
> +  later one.

The schema can and should express this. IOW, either move this into the 
system controller schema or reference this ($ref) from it.

> +
> +properties:
> +  compatible:
> +    const: baikal,bt1-ccu-pll
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    description: External reference clock
> +    maxItems: 1
> +
> +  clock-names:
> +    const: ref_clk
> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +
> +examples:
> +  # Clock Control Unit PLL node:
> +  - |
> +    clock-controller-pll {
> +      compatible = "baikal,bt1-ccu-pll";
> +      #clock-cells = <1>;
> +
> +      clocks = <&clk25m>;
> +      clock-names = "ref_clk";

If there's a register range within the system controller for the pll, 
then add 'reg' even if Linux doesn't use it.

> +    };
> +  # Required external oscillator:
> +  - |
> +    clk25m: clock-oscillator-25m {
> +      compatible = "fixed-clock";
> +      #clock-cells = <0>;
> +      clock-frequency  = <25000000>;
> +      clock-output-names = "clk25m";
> +    };
> +...
> diff --git a/include/dt-bindings/clock/bt1-ccu.h b/include/dt-bindings/clock/bt1-ccu.h
> new file mode 100644
> index 000000000000..931a4bea67c0
> --- /dev/null
> +++ b/include/dt-bindings/clock/bt1-ccu.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> + *
> + * Baikal-T1 CCU clock indices
> + */
> +#ifndef __DT_BINDINGS_CLOCK_BT1_CCU_H
> +#define __DT_BINDINGS_CLOCK_BT1_CCU_H
> +
> +#define CCU_CPU_PLL			0
> +#define CCU_SATA_PLL			1
> +#define CCU_DDR_PLL			2
> +#define CCU_PCIE_PLL			3
> +#define CCU_ETH_PLL			4
> +
> +#endif /* __DT_BINDINGS_CLOCK_BT1_CCU_H */
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists