Re: [PATCH] dt-bindings: clock: brcm,kona-ccu: convert to YAML

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 22/10/2023 13:31, Stanislav Jakubek wrote:
> Convert Broadcom Kona family clock controller unit (CCU) bindings
> to DT schema.
> 
> Signed-off-by: Stanislav Jakubek <stano.jakubek@gmail.com>

Thank you for your patch. There is something to discuss/improve.

> +description:
> +  Broadcom "Kona" style clock control unit (CCU) is a clock provider that
> +  manages a set of clock signals.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,bcm11351-aon-ccu
> +      - brcm,bcm11351-hub-ccu
> +      - brcm,bcm11351-master-ccu
> +      - brcm,bcm11351-root-ccu
> +      - brcm,bcm11351-slave-ccu
> +      - brcm,bcm21664-aon-ccu
> +      - brcm,bcm21664-master-ccu
> +      - brcm,bcm21664-root-ccu
> +      - brcm,bcm21664-slave-ccu
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  clock-output-names:
> +    minItems: 1
> +    maxItems: 10
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - brcm,bcm11351-aon-ccu
> +              - brcm,bcm11351-hub-ccu
> +              - brcm,bcm11351-master-ccu
> +              - brcm,bcm11351-root-ccu
> +              - brcm,bcm11351-slave-ccu
> +    then:
> +      properties:
> +        clock-output-names:
> +          description: |
> +            The following table defines the set of CCUs and clock specifiers
> +            for BCM281XX family clocks.
> +            These clock specifiers are defined in:
> +                "include/dt-bindings/clock/bcm281xx.h"
> +
> +            CCU     Clock        Type  Index  Specifier
> +            ---     -----        ----  -----  ---------
> +            root    frac_1m      peri    0    BCM281XX_ROOT_CCU_FRAC_1M
> +
> +            aon     hub_timer    peri    0    BCM281XX_AON_CCU_HUB_TIMER
> +            aon     pmu_bsc      peri    1    BCM281XX_AON_CCU_PMU_BSC
> +            aon     pmu_bsc_var  peri    2    BCM281XX_AON_CCU_PMU_BSC_VAR
> +
> +            hub     tmon_1m      peri    0    BCM281XX_HUB_CCU_TMON_1M
> +
> +            master  sdio1        peri    0    BCM281XX_MASTER_CCU_SDIO1
> +            master  sdio2        peri    1    BCM281XX_MASTER_CCU_SDIO2
> +            master  sdio3        peri    2    BCM281XX_MASTER_CCU_SDIO3
> +            master  sdio4        peri    3    BCM281XX_MASTER_CCU_SDIO4
> +            master  dmac         peri    4    BCM281XX_MASTER_CCU_DMAC
> +            master  usb_ic       peri    5    BCM281XX_MASTER_CCU_USB_IC
> +            master  hsic2_48m    peri    6    BCM281XX_MASTER_CCU_HSIC_48M
> +            master  hsic2_12m    peri    7    BCM281XX_MASTER_CCU_HSIC_12M
> +
> +            slave   uartb        peri    0    BCM281XX_SLAVE_CCU_UARTB
> +            slave   uartb2       peri    1    BCM281XX_SLAVE_CCU_UARTB2
> +            slave   uartb3       peri    2    BCM281XX_SLAVE_CCU_UARTB3
> +            slave   uartb4       peri    3    BCM281XX_SLAVE_CCU_UARTB4
> +            slave   ssp0         peri    4    BCM281XX_SLAVE_CCU_SSP0
> +            slave   ssp2         peri    5    BCM281XX_SLAVE_CCU_SSP2
> +            slave   bsc1         peri    6    BCM281XX_SLAVE_CCU_BSC1
> +            slave   bsc2         peri    7    BCM281XX_SLAVE_CCU_BSC2
> +            slave   bsc3         peri    8    BCM281XX_SLAVE_CCU_BSC3
> +            slave   pwm          peri    9    BCM281XX_SLAVE_CCU_PWM

I don't really understand why this is in the binding schema. I guess you
wanted to copy it from the old binding, but, unless there is real reason
for it, don't. The clock IDs should be in the header file and that's it.
Nothing here.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - brcm,bcm21664-aon-ccu
> +              - brcm,bcm21664-master-ccu
> +              - brcm,bcm21664-root-ccu
> +              - brcm,bcm21664-slave-ccu
> +    then:
> +      properties:
> +        clock-output-names:
> +          maxItems: 8
> +          description: |
> +            The following table defines the set of CCUs and clock specifiers
> +            for BCM21664 family clocks.
> +            These clock specifiers are defined in:
> +                "include/dt-bindings/clock/bcm21664.h"
> +
> +            CCU     Clock         Type  Index  Specifier
> +            ---     -----         ----  -----  ---------
> +            root    frac_1m       peri    0    BCM21664_ROOT_CCU_FRAC_1M
> +
> +            aon     hub_timer     peri    0    BCM21664_AON_CCU_HUB_TIMER
> +
> +            master  sdio1         peri    0    BCM21664_MASTER_CCU_SDIO1
> +            master  sdio2         peri    1    BCM21664_MASTER_CCU_SDIO2
> +            master  sdio3         peri    2    BCM21664_MASTER_CCU_SDIO3
> +            master  sdio4         peri    3    BCM21664_MASTER_CCU_SDIO4
> +            master  sdio1_sleep   peri    4    BCM21664_MASTER_CCU_SDIO1_SLEEP
> +            master  sdio2_sleep   peri    5    BCM21664_MASTER_CCU_SDIO2_SLEEP
> +            master  sdio3_sleep   peri    6    BCM21664_MASTER_CCU_SDIO3_SLEEP
> +            master  sdio4_sleep   peri    7    BCM21664_MASTER_CCU_SDIO4_SLEEP
> +
> +            slave   uartb         peri    0    BCM21664_SLAVE_CCU_UARTB
> +            slave   uartb2        peri    1    BCM21664_SLAVE_CCU_UARTB2
> +            slave   uartb3        peri    2    BCM21664_SLAVE_CCU_UARTB3
> +            slave   uartb4        peri    3    BCM21664_SLAVE_CCU_UARTB4
> +            slave   bsc1          peri    4    BCM21664_SLAVE_CCU_BSC1
> +            slave   bsc2          peri    5    BCM21664_SLAVE_CCU_BSC2
> +            slave   bsc3          peri    6    BCM21664_SLAVE_CCU_BSC3
> +            slave   bsc4          peri    7    BCM21664_SLAVE_CCU_BSC4

Same comments.

In any case, allOf: goes after required: block.

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +  - clock-output-names
> +
> +additionalProperties: false
> +
Best regards,
Krzysztof