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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4649 bytes --]

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);
}


> > > +	} 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.

> > > +	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.

> > > +	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.


-- 
 i.