Re: [PATCH 1/6] dt-bindings: clock: imx8m-anatop: support spread spectrum clocking

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/10/2024 10:57, Dario Binacchi wrote:
> On Thu, Oct 3, 2024 at 12:46 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 01/10/2024 08:29, Dario Binacchi wrote:
>>> On Mon, Sep 30, 2024 at 8:45 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>> On 29/09/2024 22:00, Dario Binacchi wrote:
>>>>>>
>>>>>>
>>>>>>> +  properties:
>>>>>>> +    compatible:
>>>>>>> +      contains:
>>>>>>> +        enum:
>>>>>>> +          - fsl,imx8mm-anatop
>>>>>>> +
>>>>>>> +then:
>>>>>>> +  properties:
>>>>>>> +    fsl,ssc-clocks:
>>>>>>
>>>>>> Nope. Properties must be defined in top-level.
>>>>>>
>>>>>>> +      $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>> +      description:
>>>>>>> +        The phandles to the PLLs with spread spectrum clock generation
>>>>>>> +        hardware capability.
>>>>>>
>>>>>> These should be clocks.
>>>>>
>>>>> Sorry, but I can't understand what you're asking me.
>>>>> Could you kindly explain it to me in more detail?
>>>>
>>>> You added new property instead of using existing one for this purpose:
>>>> 'clocks'.
>>>
>>>>
>>>>
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> I added this new property specifically for managing spread-spectrum.
>>> Indeed, not all clocks/PLLs
>>> managed by the node/peripheral support spread-spectrum, and the added
>>> properties specify
>>> parameters for enabling and tuning SSC for each individual PLL based
>>> on the index of each list.
>>> If I were to use the 'clocks' property and add a clock to this list
>>> that does not support SSC, IMHO
>>> the pairings would be less clear.
>>
>> You duplicate property with argument "pairings shall match". Well, I am
>> not happy with the duplication. Clocks have specific order, thus it is
>> explicit which one needs tuning. Your other properties can match them as
>> well, just index from clocks is offset...
> 
> Just to check if I understood correctly what you are suggesting before
> submitting version 3 of the patch.
> Something, for example, like:
> 
> clocks = <&clk, IMX8MP_AUDIO_PLL1>,  <&clk, IMX8MP_AUDIO_PLL2>, <&clk
> IMX8MP_VIDEO_PLL1>;
> fsl,ssc-modfreq-hz = <0, 3517>, <2, 6818>;

Hm, what is 0? If clock index, then no, it's redundant. The first item
in cannot point to other clock.

Also, what exactly are you setting here and why assigned-clock-rates are
not working?

Best regards,
Krzysztof