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

[Dario Binacchi]

On Thu, Oct 3, 2024 at 12:46 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 01/10/2024 08:29, Dario Binacchi wrote:
> > On Mon, Sep 30, 2024 at 8:45 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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>;

Where the spread spectrum is enabled only for AUDIO_PLL1 and VIDEO_PLL1 and
not for IMX8MP_AUDIO_PLL2.

Is this what you meant?

Thanks and regards,
Dario

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


-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@xxxxxxxxxxxxxxxxxxxx

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@xxxxxxxxxxxxxxxxxxxx

www.amarulasolutions.com