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

[Krzysztof Kozlowski <krzk@kernel.org>]

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


> 
> AFAIK the confusion arises from the fact that this node, which is a
> clock controller, was used only
> to export its base address, but perhaps it should have also exported
> its clocks, which the other
> clock controller does, as shown in:
> Documentation/devicetree/bindings/clock/imx8m-clock.yaml.

You use it as clocks, so I don't understand this comment.

> If I consider its 'compatible' entries:
> - 'fsl,imx8mm-ccm' -> drivers/clk/imx/clk-imx8mm.c
> - 'fsl,imx8mn-ccm' -> drivers/clk/imx/clk-imx8mn.c
> - 'fsl,imx8mp-ccm' -> drivers/clk/imx/clk-imx8mp.c
> the probe function, triggered by fsl,imx8m{m,n,p}-ccm (and not
> fsl,imx8m{m,n,p}-anatop),
> retrieves the anatop node solely to get its base address, also
> registering its clocks, which
> I would have expected to be registered by another driver, specifically
> the one for anatop:
> 
> static int imx8mn_clocks_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> void __iomem *base;
> struct imx_pll14xx_ssc pll1443x_ssc;
> int ret;
> 
> clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws,
>                            IMX8MN_CLK_END), GFP_KERNEL);
> if (WARN_ON(!clk_hw_data))
>     return -ENOMEM;
> 
> clk_hw_data->num = IMX8MN_CLK_END;
> hws = clk_hw_data->hws;
> 
> hws[IMX8MN_CLK_DUMMY] = imx_clk_hw_fixed("dummy", 0);
> hws[IMX8MN_CLK_24M] = imx_get_clk_hw_by_name(np, "osc_24m");
> hws[IMX8MN_CLK_32K] = imx_get_clk_hw_by_name(np, "osc_32k");
> hws[IMX8MN_CLK_EXT1] = imx_get_clk_hw_by_name(np, "clk_ext1");
> hws[IMX8MN_CLK_EXT2] = imx_get_clk_hw_by_name(np, "clk_ext2");
> hws[IMX8MN_CLK_EXT3] = imx_get_clk_hw_by_name(np, "clk_ext3");
> hws[IMX8MN_CLK_EXT4] = imx_get_clk_hw_by_name(np, "clk_ext4");
> 
> np = of_find_compatible_node(NULL, NULL, "fsl,imx8mn-anatop");
> base = devm_of_iomap(dev, np, 0, NULL);
> of_node_put(np);
> if (WARN_ON(IS_ERR(base))) {
>     ret = PTR_ERR(base);
>     goto unregister_hws;
> }
> 
> hws[IMX8MN_AUDIO_PLL1_REF_SEL] = imx_clk_hw_mux("audio_pll1_ref_sel",
> base + 0x0, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
> hws[IMX8MN_AUDIO_PLL2_REF_SEL] = imx_clk_hw_mux("audio_pll2_ref_sel",
> base + 0x14, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
> hws[IMX8MN_VIDEO_PLL_REF_SEL] = imx_clk_hw_mux("video_pll_ref_sel",
> base + 0x28, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));

Sorry, I am not going to dwell into drivers code. We talk here about
bindings and new properties.

Best regards,
Krzysztof