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
