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

Krzysztof Kozlowski
2024-10-06T13:13:52Z

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