Re: [PATCH 12/15] clk: nuvoton: Add clock driver for ma35d1 clock controller

[Jacky Huang <ychuang570808@gmail.com>]

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@nuvoton.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@nuvoton.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@nuvoton.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