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

[Yixun Lan]

HI Jerome:

On 12/11/17 17:46, Jerome Brunet wrote:
> 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
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..

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


>> +	},
>> +};
> 
> [...]
> 
>> +/*
>> + * 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

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