HI Jerome:

On 12/11/17 17:46, Jerome Brunet wrote:

On Mon, 2017-12-11 at 14:48 +0800, Yixun Lan wrote:

quoted

From: Qiufang Dai <redacted>

Add clock controller drivers for Amlogic Meson-AXG SoC.

Acked-by: Neil Armstrong <redacted>
Signed-off-by: Qiufang Dai <redacted>
Signed-off-by: Yixun Lan <redacted>
---
 arch/arm64/Kconfig.platforms |   1 +
 drivers/clk/meson/Kconfig    |   8 +
 drivers/clk/meson/Makefile   |   1 +
 drivers/clk/meson/axg.c      | 944
+++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/axg.h      | 126 ++++++
 5 files changed, 1080 insertions(+)
 create mode 100644 drivers/clk/meson/axg.c
 create mode 100644 drivers/clk/meson/axg.h

Overall, the changes looks good to me.
I still have a few comments which I'd like to see addressed quickly after this
series is merged

did you say that:
 you will merge these patches now, then I could send fixes later?

or I can send another version (v7) to address this..

[...]

quoted

+static struct meson_clk_pll axg_fixed_pll = {
+	.m = {
+		.reg_off = HHI_MPLL_CNTL,
+		.shift   = 0,
+		.width   = 9,
+	},
+	.n = {
+		.reg_off = HHI_MPLL_CNTL,
+		.shift   = 9,
+		.width   = 5,
+	},
+	.od = {
+		.reg_off = HHI_MPLL_CNTL,
+		.shift   = 16,
+		.width   = 2,
+	},
+	.lock = &clk_lock,
+	.hw.init = &(struct clk_init_data){
+		.name = "fixed_pll",
+		.ops = &meson_clk_pll_ro_ops,
+		.parent_names = (const char *[]){ "xtal" },
+		.num_parents = 1,
+		.flags = CLK_GET_RATE_NOCACHE,

Is the rate of this clock supposed to changed by something else than
the CCF after boot ?

If not, you should drop this flag.
Same comment goes for axg_sys_pll and axg_gp0_pll

we need to keep the CLK_GET_RATE_NOCACHE flag for axg_sys_pll, since the
BL30 will change it for the frequency change.
for the other two, we can drop them safely

quoted

+	},
+};

[...]

quoted

+/*
+ * FIXME The legacy composite clocks (e.g. clk81) are both PLL post-dividers
+ * and should be modeled with their respective PLLs via the forthcoming
+ * coordinated clock rates feature
+ */
+static u32 mux_table_clk81[]	= { 0, 2, 3, 4, 5, 6, 7 };
+static const char * const clk81_parent_names[] = {
+	"xtal", "fclk_div7", "mpll1", "mpll2", "fclk_div4",
+	"fclk_div3", "fclk_div5"
+};
+
+static struct clk_mux axg_mpeg_clk_sel = {
+	.reg = (void *)HHI_MPEG_CLK_CNTL,
+	.mask = 0x7,
+	.shift = 12,
+	.flags = CLK_MUX_READ_ONLY,
+	.table = mux_table_clk81,
+	.lock = &clk_lock,
+	.hw.init = &(struct clk_init_data){
+		.name = "mpeg_clk_sel",
+		.ops = &clk_mux_ro_ops,
+		/*
+		 * bits 14:12 selects from 8 possible parents:
+		 * xtal, 1'b0 (wtf), fclk_div7, mpll_clkout1, mpll_clkout2,
+		 * fclk_div4, fclk_div3, fclk_div5
+		 */

This comment is not necessary with table above.

can drop

quoted

+		.parent_names = clk81_parent_names,
+		.num_parents = ARRAY_SIZE(clk81_parent_names),
+		.flags = CLK_SET_RATE_NO_REPARENT,

With ro_ops, this flag is not necessary, consider removing it.

can drop

quoted

+	},
+};
+

.