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

[Dario Binacchi <dario.binacchi@amarulasolutions.com>]

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.

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@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com