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. 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. 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)); Thanks and regards, Dario -- 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