Re: [PATCH v5 15/20] dt-bindings: clock: imx8m-clock: support spread spectrum clocking

[Krzysztof Kozlowski]

On Thu, Dec 05, 2024 at 12:17:50PM +0100, Dario Binacchi wrote:
> The patch adds the DT bindings for enabling and tuning spread spectrum
> clocking generation.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@xxxxxxxxxxxxxxxxxxxx>
> 
> ---
> 
> (no changes since v4)
> 
> Changes in v4:
> - Drop "fsl,ssc-clocks" property. The other added properties now refer
>   to the clock list.
> - Updated minItems and maxItems of
>   - clocks
>   - clock-names
>   - fsl,ssc-modfreq-hz
>   - fsl,ssc-modrate-percent
>   - fsl,ssc-modmethod
> - Updated the dts examples
> 
> Changes in v3:
> - Added in v3
> - The dt-bindings have been moved from fsl,imx8m-anatop.yaml to
>   imx8m-clock.yaml. The anatop device (fsl,imx8m-anatop.yaml) is
>   indeed more or less a syscon, so it represents a memory area
>   accessible by ccm (imx8m-clock.yaml) to setup the PLLs.
> 
> Changes in v2:
> - Add "allOf:" and place it after "required:" block, like in the
>   example schema.
> - Move the properties definition to the top-level.
> - Drop unit types as requested by the "make dt_binding_check" command.
> 
>  .../bindings/clock/imx8m-clock.yaml           | 77 +++++++++++++++++--
>  1 file changed, 71 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> index c643d4a81478..83036f6d2274 100644
> --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> @@ -29,12 +29,12 @@ properties:
>      maxItems: 2
>  
>    clocks:
> -    minItems: 6
> -    maxItems: 7
> +    minItems: 7
> +    maxItems: 10

ABI break without mentioning, without any explanation in the commit msg.

>  
>    clock-names:
> -    minItems: 6
> -    maxItems: 7
> +    minItems: 7
> +    maxItems: 10
>  
>    '#clock-cells':
>      const: 1
> @@ -43,6 +43,34 @@ properties:
>        ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h
>        for the full list of i.MX8M clock IDs.
>  
> +  fsl,ssc-modfreq-hz:
> +    description:
> +      The values of modulation frequency (Hz unit) for each clock
> +      supporting spread spectrum.
> +    minItems: 7
> +    maxItems: 10

Why all cloks receive now spread spectrum? I had impression - and all
your previous versions were doing this - that you have only three or
four clocks with SSC.

Do existing clocks 1-6 support SSC?

> +
> +  fsl,ssc-modrate-percent:
> +    description:
> +      The percentage values of modulation rate for each clock
> +      supporting spread spectrum.
> +    minItems: 7
> +    maxItems: 10
> +
> +  fsl,ssc-modmethod:
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +    description:
> +      The modulation techniques for each clock supporting spread
> +      spectrum.
> +    minItems: 7
> +    maxItems: 10
> +    items:
> +      enum:
> +        - ""

Drop "", not sure why do you need it.

> +        - down-spread
> +        - up-spread
> +        - center-spread
> +
>  required:
>    - compatible
>    - reg
> @@ -76,6 +104,10 @@ allOf:
>              - const: clk_ext2
>              - const: clk_ext3
>              - const: clk_ext4
> +        fsl,ssc-modfreq-hz: false
> +        fsl,ssc-modrate-percent: false
> +        fsl,ssc-modmethod: false
> +
>      else:
>        properties:
>          clocks:
> @@ -86,6 +118,10 @@ allOf:
>              - description: ext2 clock input
>              - description: ext3 clock input
>              - description: ext4 clock input
> +            - description: audio1 PLL input
> +            - description: audio2 PLL input
> +            - description: dram PLL input
> +            - description: video PLL input

Also ABI break....

Best regards,
Krzysztof