linux-kernel - Re: [PATCH v1 2/3] clk: starfive: Add StarFive JH7110 PLL clock driver

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]

Date:   Thu, 23 Feb 2023 11:10:31 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Xingyu Wu <xingyu.wu@...rfivetech.com>
Cc:     linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Emil Renner Berthing <kernel@...il.dk>,
        Rob Herring <robh+dt@...nel.org>,
        Conor Dooley <conor@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Hal Feng <hal.feng@...rfivetech.com>,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v1 2/3] clk: starfive: Add StarFive JH7110 PLL clock
 driver

On 23/02/2023 11:03, Xingyu Wu wrote:
> On 2023/2/23 17:35, Krzysztof Kozlowski wrote:
>> On 23/02/2023 10:32, Xingyu Wu wrote:
>>> On 2023/2/23 16:56, Krzysztof Kozlowski wrote:
>>>> On 21/02/2023 15:11, Xingyu Wu wrote:
>>>>> Add driver for the StarFive JH7110 PLL clock controller and
>>>>> modify the JH7110 system clock driver to rely on this PLL clocks.
>>>>>
>>>>> Signed-off-by: Xingyu Wu <xingyu.wu@...rfivetech.com>
>>>>> ---
>>>>
>>>>
>>>>> +
>>>>> +static int jh7110_pll_clk_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	int ret;
>>>>> +	struct of_phandle_args args;
>>>>> +	struct regmap *pll_syscon_regmap;
>>>>> +	unsigned int idx;
>>>>> +	struct jh7110_clk_pll_priv *priv;
>>>>> +	struct jh7110_clk_pll_data *data;
>>>>> +	char *pll_name[JH7110_PLLCLK_END] = {
>>>>> +		"pll0_out",
>>>>> +		"pll1_out",
>>>>> +		"pll2_out"
>>>>> +	};
>>>>> +
>>>>> +	priv = devm_kzalloc(&pdev->dev,
>>>>> +			    struct_size(priv, data, JH7110_PLLCLK_END),
>>>>> +			    GFP_KERNEL);
>>>>> +	if (!priv)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	priv->dev = &pdev->dev;
>>>>> +	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node, "starfive,sysreg", 0, 0, &args);
>>>>
>>>> 1. Wrong wrapping. Wrap code at 80 as coding style asks.
>>>>
>>>> 2. Why you are using syscon for normal, device MMIO operation? Your DTS
>>>> also points that this is incorrect, hacky representation of hardware.
>>>> Don't add devices to DT to fake places and then overuse syscon to fix
>>>> that fake placement. The clock is in system registers, thus it must be
>>>> there.
>>>>
>>>> 3. Even if this stays, why so complicated code instead of
>>>> syscon_regmap_lookup_by_phandle()?
>>>>
>>>
>>> Thanks for your advice. Will use syscon_regmap_lookup_by_phandle instead it
>>> and remove useless part.
>>
>> So you ignored entirely part 2? This was the main comment... I am going
>> to keep NAK-ing it then.
> 
> What I understand to mean is that I cannot use a fake node to operate syscon
> registers. So I should move the PLL node under syscon node directly. Is it ok?

Yes, because it looks like entire PLL clock control is from the syscon
node, thus the clocks are there.

Best regards,
Krzysztof