lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3df06d79-ea51-4202-8cc8-468f741603bf@linaro.org>
Date:   Mon, 23 Oct 2023 09:54:49 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Stanislav Jakubek <stano.jakubek@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>
Cc:     bcm-kernel-feedback-list@...adcom.com, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Artur Weber <aweber.kernel@...il.com>
Subject: Re: [PATCH] dt-bindings: clock: brcm,kona-ccu: convert to YAML

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@...il.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ