On Mon, Oct 23, 2023 at 09:54:49AM +0200, Krzysztof Kozlowski wrote: > 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@xxxxxxxxx> > > 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. Hi Krzysztof, you're correct that I just copied this from the old bindings. brcm,iproc-clocks.yaml has a similar table, so I thought this would be fine. I'm OK with dropping it, but how should I document the clock-output-names values then? A bunch of if-then blocks (per compatible)? Or should I not even bother and just keep minItems/maxItems without documenting the values? > > > + - 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 I've also noticed that dtbs_check gives out warnings(?) like this for bcm21664 ccu nodes: /arch/arm/boot/dts/broadcom/bcm21664-garnet.dtb: root_ccu@35001000: clock-output-names: ['frac_1m'] is too short from schema $id: http://devicetree.org/schemas/clock/brcm,kona-ccu.yaml# and this maxItems:8 seems to me like the culprit (since the bcm11351 if-then doesn't have that). Seems to me like it also overrides the minItems to be 8 as well. I don't understand why it would do that though. I suppose just adding minItems: 1 would be the correct fix in this case? > > + 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. Ack. > > > + > > +required: > > + - compatible > > + - reg > > + - '#clock-cells' > > + - clock-output-names > > + > > +additionalProperties: false > > + > Best regards, > Krzysztof > Thanks for the feedback, Stanislav