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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <82db5037-bbd3-4005-bde9-02df1bf4c475@kernel.org>
Date: Thu, 3 Oct 2024 12:46:13 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Dario Binacchi <dario.binacchi@...rulasolutions.com>
Cc: linux-kernel@...r.kernel.org, linux-amarula@...rulasolutions.com,
 Conor Dooley <conor+dt@...nel.org>, Fabio Estevam <festevam@...il.com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Michael Turquette <mturquette@...libre.com>, Peng Fan <peng.fan@....com>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Rob Herring <robh@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
 Shawn Guo <shawnguo@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
 devicetree@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH 1/6] dt-bindings: clock: imx8m-anatop: support spread
 spectrum clocking

On 01/10/2024 08:29, Dario Binacchi wrote:
> On Mon, Sep 30, 2024 at 8:45 AM Krzysztof Kozlowski <krzk@...nel.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