| 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
| ||
|
Date: Tue, 21 Mar 2023 23:03:12 +0800
From: Jacky Huang <ychuang570808@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
lee@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
p.zabel@...gutronix.de,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, devicetree@...r.kernel.org,
linux-clk@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
linux-serial <linux-serial@...r.kernel.org>, schung@...oton.com,
Jacky Huang <ychuang3@...oton.com>
Subject: Re: [PATCH 12/15] clk: nuvoton: Add clock driver for ma35d1 clock
controller
Dear Ilpo,
On 2023/3/20 下午 06:31, Ilpo Järvinen wrote:
> On Sun, 19 Mar 2023, Jacky Huang wrote:
>
>> On 2023/3/16 下午 11:56, Ilpo Järvinen wrote:
>>> On Wed, 15 Mar 2023, Jacky Huang wrote:
>>>
>>>> From: Jacky Huang <ychuang3@...oton.com>
>>>>
>>>> The clock controller generates clocks for the whole chip, including
>>>> system clocks and all peripheral clocks. This driver support ma35d1
>>>> clock gating, divider, and individual PLL configuration.
>>>>
>>>> There are 6 PLLs in ma35d1 SoC:
>>>> - CA-PLL for the two Cortex-A35 CPU clock
>>>> - SYS-PLL for system bus, which comes from the companion MCU
>>>> and cannot be programmed by clock controller.
>>>> - DDR-PLL for DDR
>>>> - EPLL for GMAC and GFX, Display, and VDEC IPs.
>>>> - VPLL for video output pixel clock
>>>> - APLL for SDHC, I2S audio, and other IPs.
>>>> CA-PLL has only one operation mode.
>>>> DDR-PLL, EPLL, VPLL, and APLL are advanced PLLs which have 3
>>>> operation modes: integer mode, fraction mode, and spread specturm mode.
>>>>
>>>> Signed-off-by: Jacky Huang <ychuang3@...oton.com>
>>>> ---
>>>> +};
>>>> +
>>>> +#define to_ma35d1_adc_clk_divider(_hw) \
>>>> + container_of(_hw, struct ma35d1_adc_clk_divider, hw)
>>> static inline
>>
>> I will modify these "static" functions as "static inline".
> No, that's not what I meant. Make the container_of define static inline
> function instead, no other functions. (Or if you have more than one of
> such, all of them of course).
>
>>>> +}
>>>> diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c
>>>> b/drivers/clk/nuvoton/clk-ma35d1-pll.c
>>>> new file mode 100644
>>>> index 000000000000..79e724b148fa
>>>> --- /dev/null
>>>> +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c
>>>> @@ -0,0 +1,534 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (C) 2023 Nuvoton Technology Corp.
>>>> + * Author: Chi-Fang Li <cfli0@...oton.com>
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/clk-provider.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/bitfield.h>
>>>> +
>>>> +#include "clk-ma35d1.h"
>>>> +
>>>> +#define to_ma35d1_clk_pll(clk) \
>>>> + (container_of(clk, struct ma35d1_clk_pll, clk))
>>> static inline
>>
>> I am sorry cannot get "static inline" refer to which one.
>>
>> Would you give more advice here?
>>
>> Thank you.
> static inline struct ...type_here... *to_ma35d1_clk_pll(struct ...type_here... *clk)
> {
> return container_of(clk, struct ma35d1_clk_pll, clk);
> }
>
OK, I got it. Thank you very much.
>>>> + } else {
>>>> + pr_err("Failed to set rate %ld\n", u64PllFreq);
>>>> + return 0;
>>>> + }
>>>> +
>>>> + u64P = (u64FCLKO >= VSIPLL_FCLK_MIN_FREQ) ? 1 :
>>>> + ((VSIPLL_FCLK_MIN_FREQ / u64FCLKO) +
>>>> + ((VSIPLL_FCLK_MIN_FREQ % u64FCLKO) ? 1 : 0));
>>> Ditto.
>>>
>>> Is here some ...ROUND_UP() trick hidden too?
>>
>> This follows the description of PLL spec.
> Right but I was looking into what the math does. To me this looks like
> rounding up:
> VSIPLL_FCLK_MIN_FREQ / u64FCLKO + (VSIPLL_FCLK_MIN_FREQ % u64FCLKO ? 1 : 0)
>
> When modulo is > 0, add one, which is round up, no?
>
> There are helpers which you should use for rounding up, search for
> *_ROUND_UP. I think math64.h had one 64-bit one.
Yes, it is a round up. We will find out all the occurrence and use
ROUND_UP() macro instead.
>>>> + u64X = u64tmp % 1000;
>>>> + u32FRAC = ((u64X << 24) + 500) / 1000;
> I missed this earlier, is this rounding? ...Use a helper if it is.
> Otherwise define what 500 is. (No need to answer despite question mark,
> just do the change).
>
>>>> +
>>>> + u64SSRATE = ((PllSrcClk >> 1) / (u32Fmod * 2)) - 1;
>>>> + u64SLOPE = ((u64tmp * u32SR / u64SSRATE) << 24) / 100 / 1000;
>>>> +
>>>> + u64PllClk = (PllSrcClk * u64tmp) / u64P / u64M / 1000;
>>> Is some *SEC_PER_*SEC define relevant for 1000 ?
>>>
>>> Or some other units, e.g., HZ related?
>>
>> 1000 is for kHz to MHz, and 100 is for percentage.
> Okay, then use KHZ_PER_MHZ from linux/units.h.
>
> We don't have anything for percents under include/ I think so that can be
> left as literal.
Sure, we are rewriting the pll calculation routine and add formula
comments to make it more readable.
>>>> + switch (pll->mode) {
>>>> + case VSIPLL_INTEGER_MODE:
>>>> + u64PllClk = CLK_CalPLLFreq_Mode0(PllSrcClk, u64PllFreq,
>>>> + u32Reg);
>>> One line.
>>
>> It will exceed 80 characters in one line.
> Yeah, the semicolon won't fit to 80 chars :-) which means there won't be
> significant information loss even on 80 chars terminal. This kind of cases
> is why checkpatch won't complain until 100 chars. Use common sense (don't
> hide most of the logic to 80-100 but don't be afraid of breaking the 80
> chars where the information loss is not significant issue).
>
> Besides, once you removed the types from variable names, it will be
> shorter anyway.
>
Got it. Thanks for your kind help and detailed explanation.
Best regards,
Jacky Huang
Powered by blists - more mailing lists