Re: [PATCH v1 1/3] dt-bindings: clock: Add Exynos8895 SoC CMU bindings

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/10/2024 19:48, Ivaylo Ivanov wrote:
> Provide dt-schema documentation for Exynos8895 SoC clock controller.
> Add device tree clock binding definitions for the following CMU blocks:
>  - CMU_TOP
>  - CMU_FSYS0/1
>  - CMU_PERIC0/1
>  - CMU_PERIS
> 
> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>


A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> +
> +title: Samsung Exynos8895 SoC clock controller
> +
> +maintainers:
> +  - Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
> +  - Chanwoo Choi <cw00.choi@samsung.com>
> +  - Krzysztof Kozlowski <krzk@kernel.org>
> +  - Sylwester Nawrocki <s.nawrocki@samsung.com>
> +  - Tomasz Figa <tomasz.figa@gmail.com>

Please drop Sylwester and Tomasz, they opted out from clocks.

> +
> +description: |
> +  Exynos8895 clock controller is comprised of several CMU units, generating
> +  clocks for different domains. Those CMU units are modeled as separate device
> +  tree nodes, and might depend on each other. The root clock in that root tree
> +  is an external clock: OSCCLK (26 MHz). This external clock must be defined
> +  as a fixed-rate clock in dts.
> +
> +  CMU_TOP is a top-level CMU, where all base clocks are prepared using PLLs and
> +  dividers; all other clocks of function blocks (other CMUs) are usually
> +  derived from CMU_TOP.
> +
> +  Each clock is assigned an identifier and client nodes can use this identifier
> +  to specify the clock which they consume. All clocks available for usage
> +  in clock consumer nodes are defined as preprocessor macros in
> +  'include/dt-bindings/clock/samsung,exynos8895.h' header.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - samsung,exynos8895-cmu-top
> +      - samsung,exynos8895-cmu-fsys0
> +      - samsung,exynos8895-cmu-fsys1
> +      - samsung,exynos8895-cmu-peric0
> +      - samsung,exynos8895-cmu-peric1
> +      - samsung,exynos8895-cmu-peris

Alphabetical order.

> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 16
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 16
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +

required: block should go here (I know that other Samsung clock bindings
do not follow this convention).

> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos8895-cmu-top
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos8895-cmu-fsys0
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_FSYS0 BUS clock (from CMU_TOP)
> +            - description: CMU_FSYS0 DPGTC clock (from CMU_TOP)
> +            - description: CMU_FSYS0 MMC_EMBD clock (from CMU_TOP)
> +            - description: CMU_FSYS0 UFS_EMBD clock (from CMU_TOP)
> +            - description: CMU_FSYS0 USBDRD30 clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: bus
> +            - const: dpgtc
> +            - const: mmc_embd

mmc

> +            - const: ufs_embd

ufs

> +            - const: usbdrd30
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos8895-cmu-fsys1
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_FSYS1 BUS clock (from CMU_TOP)
> +            - description: CMU_FSYS1 MMC_CARD clock (from CMU_TOP)
> +            - description: CMU_FSYS1 PCIE clock (from CMU_TOP)
> +            - description: CMU_FSYS1 UFS_CARD clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: bus
> +            - const: mmc_card

mmc
Although now I wonder, why this is different FSYS. Is it for different
mmc controller?

> +            - const: pcie
> +            - const: ufs_card

ufs

Keep the order as in GS101 file.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos8895-cmu-peric0
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_PERIC0 BUS clock (from CMU_TOP)
> +            - description: CMU_PERIC0 UART_DBG clock (from CMU_TOP)
> +            - description: CMU_PERIC0 USI00 clock (from CMU_TOP)
> +            - description: CMU_PERIC0 USI01 clock (from CMU_TOP)
> +            - description: CMU_PERIC0 USI02 clock (from CMU_TOP)
> +            - description: CMU_PERIC0 USI03 clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: bus
> +            - const: uart_dbg

uart

> +            - const: usi00

usi0

> +            - const: usi01

usi1

> +            - const: usi02

usi2

> +            - const: usi03

usi3

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos8895-cmu-peric1
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_PERIC1 BUS clock (from CMU_TOP)
> +            - description: CMU_PERIC1 SPEEDY2 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 SPI_CAM0 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 SPI_CAM1 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 UART_BT clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI04 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI05 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI06 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI07 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI08 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI09 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI10 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI11 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI12 clock (from CMU_TOP)
> +            - description: CMU_PERIC1 USI13 clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: bus
> +            - const: speedy2

speedy

> +            - const: cam0
> +            - const: cam1
> +            - const: uart_bt

uart

> +            - const: usi04

usi4, etc

> +            - const: usi05
> +            - const: usi06
> +            - const: usi07
> +            - const: usi08
> +            - const: usi09
> +            - const: usi10
> +            - const: usi11
> +            - const: usi12
> +            - const: usi13
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos8895-cmu-peris
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_PERIS BUS clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: bus
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  # Clock controller node for CMU_FSYS1
> +  - |
> +    #include <dt-bindings/clock/samsung,exynos8895.h>
> +
> +    cmu_fsys1: clock-controller@11400000 {
> +        compatible = "samsung,exynos8895-cmu-fsys1";
> +        reg = <0x11400000 0x8000>;
> +        #clock-cells = <1>;
> +
> +        clocks = <&oscclk>,
> +                 <&cmu_top CLK_DOUT_CMU_FSYS1_BUS>,
> +                 <&cmu_top CLK_DOUT_CMU_FSYS1_MMC_CARD>,
> +                 <&cmu_top CLK_DOUT_CMU_FSYS1_PCIE>,
> +                 <&cmu_top CLK_DOUT_CMU_FSYS1_UFS_CARD>;
> +        clock-names = "oscclk", "bus", "mmc_card",
> +                      "pcie", "ufs_card";
> +    };
> +


Best regards,
Krzysztof