Re: [PATCH v6 3/6] clk: meson-axg: add clock controller drivers

[Jerome Brunet]

On Mon, 2017-12-11 at 14:48 +0800, Yixun Lan wrote:
> From: Qiufang Dai <qiufang.dai@xxxxxxxxxxx>
> 
> Add clock controller drivers for Amlogic Meson-AXG SoC.
> 
> Acked-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Signed-off-by: Qiufang Dai <qiufang.dai@xxxxxxxxxxx>
> Signed-off-by: Yixun Lan <yixun.lan@xxxxxxxxxxx>
> ---
>  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

[...]

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

> +	},
> +};

[...]

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

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

> +	},
> +};
> +
>