| 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
| ||
|
Message-ID:
<MA0P287MB0332B44AC5FC2A6FE831686EFE90A@MA0P287MB0332.INDP287.PROD.OUTLOOK.COM>
Date: Mon, 18 Dec 2023 17:01:50 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Stephen Boyd <sboyd@...nel.org>, Conor Dooley <conor.dooley@...rochip.com>
Cc: Conor Dooley <conor@...nel.org>, Chen Wang <unicornxw@...il.com>,
aou@...s.berkeley.edu, chao.wei@...hgo.com,
krzysztof.kozlowski+dt@...aro.org, mturquette@...libre.com,
palmer@...belt.com, paul.walmsley@...ive.com, richardcochran@...il.com,
robh+dt@...nel.org, devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
haijiao.liu@...hgo.com, xiaoguang.xing@...hgo.com, guoren@...nel.org,
jszhang@...nel.org, inochiama@...look.com, samuel.holland@...ive.com
Subject: Re: [PATCH v6 3/4] clk: sophgo: Add SG2042 clock generator driver
On 2023/12/14 8:15, Stephen Boyd wrote:
> Quoting Conor Dooley (2023-12-12 00:37:39)
>> On Tue, Dec 12, 2023 at 10:22:28AM +0800, Chen Wang wrote:
>>
>>> On 2023/12/9 0:47, Conor Dooley wrote:
>>>> On Fri, Dec 08, 2023 at 09:14:32AM +0800, Chen Wang wrote:
>>>>> +#define ENCODE_PLL_CTRL(fbdiv, p1, p2, refdiv) \
>>>>> + (((fbdiv & 0xfff) << 16) | ((p2 & 0x7) << 12) | ((p1 & 0x7) << 8) | (refdiv & 0x3f))
>>>> IMO this should be a function not a macro.
>>> Would like to listen why it should be a function instead of a macro? Any
>>> experiences you can share with me?
>> Readability. A function, which could be inlined allows you to break this
>> up and make it easier to read.
>>
>>>>> +/*
>>>>> + * Based on input rate/prate/fbdiv/refdiv, look up the postdiv1_2 table
>>>>> + * to get the closest postdiiv combination.
>>>>> + * @rate: FOUTPOSTDIV
>>>>> + * @prate: parent rate, i.e. FREF
>>>>> + * @fbdiv: FBDIV
>>>>> + * @refdiv: REFDIV
>>>>> + * @postdiv1: POSTDIV1, output
>>>>> + * @postdiv2: POSTDIV2, output
>>>>> + * See TRM:
>>>>> + * FOUTPOSTDIV = FREF * FBDIV / REFDIV / (POSTDIV1 * POSTDIV2)
>>>>> + * So we get following formula to get POSTDIV1 and POSTDIV2:
>>>>> + * POSTDIV = (prate/REFDIV) x FBDIV/rate
>>>>> + * above POSTDIV = POSTDIV1*POSTDIV2
>>>>> + */
>>>>> +static int __sg2042_pll_get_postdiv_1_2(
>>>>> + unsigned long rate,
>>>>> + unsigned long prate,
>>>>> + unsigned int fbdiv,
>>>>> + unsigned int refdiv,
>>>>> + unsigned int *postdiv1,
>>>>> + unsigned int *postdiv2)
>>>> This is not the coding style btw.
>>> Agree, will fix this.
>>>>> +{
>>>>> + int index = 0;
>>>>> + int ret = 0;
>>>>> + u64 tmp0;
>>>>> +
>>>>> + /* prate/REFDIV and result save to tmp0 */
>>>>> + tmp0 = prate;
>>>>> + do_div(tmp0, refdiv);
>>>>> +
>>>>> + /* ((prate/REFDIV) x FBDIV) and result save to tmp0 */
>>>>> + tmp0 *= fbdiv;
>>>>> +
>>>>> + /* ((prate/REFDIV) x FBDIV)/rate and result save to tmp0 */
>>>>> + do_div(tmp0, rate);
>>>>> +
>>>>> + /* tmp0 is POSTDIV1*POSTDIV2, now we calculate div1 and div2 value */
>>>>> + if (tmp0 <= 7) {
>>>>> + /* (div1 * div2) <= 7, no need to use array search */
>>>>> + *postdiv1 = tmp0;
>>>>> + *postdiv2 = 1;
> why not return 0 here?
>
>>>>> + } else {
> And then de-indent this?
>
>>>>> + /* (div1 * div2) > 7, use array search */
>>>>> + for (index = 0; index < ARRAY_SIZE(postdiv1_2); index++) {
>>>>> + if (tmp0 > postdiv1_2[index][POSTDIV_RESULT_INDEX]) {
>>>>> + continue;
>>>>> + } else {
>>>>> + /* found it */
>>>>> + break;
> This can also return?
>
>>>>> + }
>>>>> + }
>>>>> + if (index < ARRAY_SIZE(postdiv1_2)) {
> And this condition can be removed.
>
>>>>> + *postdiv1 = postdiv1_2[index][1];
>>>>> + *postdiv2 = postdiv1_2[index][0];
>>>>> + } else {
> This can be the default after the loop.
>
>>>>> + pr_debug("%s can not find in postdiv array!\n", __func__);
>>>>> + ret = -EINVAL;
>
> /* tmp0 is POSTDIV1*POSTDIV2, now we calculate div1 and div2 value */
> if (tmp0 <= 7) {
> /* (div1 * div2) <= 7, no need to use array search */
> *postdiv1 = tmp0;
> *postdiv2 = 1;
> return 0;
> }
>
> /* (div1 * div2) > 7, use array search */
> for (index = 0; index < ARRAY_SIZE(postdiv1_2); index++) {
> if (tmp0 > postdiv1_2[index][POSTDIV_RESULT_INDEX]) {
> continue;
> } else {
> *postdiv1 = postdiv1_2[index][1];
> *postdiv2 = postdiv1_2[index][0];
> return 0;
> }
> }
> pr_debug("%s can not find in postdiv array!\n", __func__);
> return -EINVAL;
Thanks, Stephen, I will improve this.
>>>> Reading this function it makes me wonder if (and I am far from the best
>>>> person to comment, someone like Stephen is vastly more qualified) you
>>>> should model this as several "stages", each implemented by the
>>>> "standard" clocks - like clk_divider etc. The code here is quite
>>>> complicated IMO as it seems to be trying to implement several stages of
>>>> division in one go.
>>> The objective of __sg2042_pll_get_postdiv_1_2() is straightforward: based on
>>> the formula defined by the TRM, with input rate/prate/fbdiv/refdiv, we can
>>> get the possiblle combination of POSTDIV1 and POSTDIV2 by looking up the
>>> table of postdiv1_2. We will later use it to setup the clock register.
>>>
>>> Though the codes looks a bit complicated, but accually it is calculate with
>>> the formula : POSTDIV = (prate/REFDIV) x FBDIV/rate, I just separate it into
>>> several steps to make it easy to understand, I have listed the formula in
>>> the comment on top of the function.
>> I understand what you are doing, I did something similar myself
>> previously. My suggestion/question was about using the "standard" types
>> of clock that the core provides to represent as many of the clocks in
>> this driver as is feasible.
> I would not twist the code to conform with the basic clk types. If
> possible it would be good to use the helpers for these things, but I
> wouldn't split up a clk that is a complex divider with multiple stages
> of division into the basic types just to make it fit. I say this because
> every clk takes more effort to maintain in the clk tree, it has a name,
> pointers, etc. If you can keep that self contained and logically it is
> really one clk, then go for it.
Thanks, I will double check if we can reuse "standard" types of clock as
much as possible, or just keep current way.
Powered by blists - more mailing lists