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

From: Jacky Huang
Date: Tue Mar 21 2023 - 11:03:24 EST

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@xxxxxxxxxxx>

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@xxxxxxxxxxx>
---
+};
+
+#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@xxxxxxxxxxx>
+ */
+
+#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