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

[Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>]

On 10/21/24 12:38, Krzysztof Kozlowski wrote:
> 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?

FSYS0 provides clocks for embedded storages (UFS and eMMC), whereas FSYS1

clocks external storage cards (UFS and MMC cards). As far as I can tell, there's

only one MMC controller, so perhaps it could be set to drive an eMMC or an SD

card. On retail devices, UFS_EMBD and MMC_CARD are used.

Thanks for the review, will fix the issues in the next patchset.

Best regards, Ivo.

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