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] [thread-next>] [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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ