Re: [PATCH 2/2] clk: meson: a1: add support for Amlogic A1 clock driver

[Jian Hu <jian.hu@amlogic.com>]

On 2019/9/27 21:32, Jerome Brunet wrote:
> 
> On Fri 27 Sep 2019 at 11:52, Jian Hu <jian.hu@amlogic.com> wrote:
> 
>> Hi, Jerome
>>
>> Thank you for review.
>>
>> On 2019/9/25 23:09, Jerome Brunet wrote:
>>> On Wed 25 Sep 2019 at 19:44, Jian Hu <jian.hu@amlogic.com> wrote:
>>>
>>>> The Amlogic A1 clock includes three parts:
>>>> peripheral clocks, pll clocks, CPU clocks.
>>>> sys pll and CPU clocks will be sent in next patch.
>>>>
>>>> Unlike the previous series, there is no EE/AO domain
>>>> in A1 CLK controllers.
>>>>
>>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
>>>> ---
>>>>    arch/arm64/Kconfig.platforms |    1 +
>>>>    drivers/clk/meson/Kconfig    |   10 +
>>>>    drivers/clk/meson/Makefile   |    1 +
>>>>    drivers/clk/meson/a1.c       | 2617 ++++++++++++++++++++++++++++++++++++++++++
>>>>    drivers/clk/meson/a1.h       |  172 +++
>>>>    5 files changed, 2801 insertions(+)
>>>>    create mode 100644 drivers/clk/meson/a1.c
>>>>    create mode 100644 drivers/clk/meson/a1.h
>>>>
>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>> index 16d7614..a48f67d 100644
>>>> --- a/arch/arm64/Kconfig.platforms
>>>> +++ b/arch/arm64/Kconfig.platforms
>>>> @@ -138,6 +138,7 @@ config ARCH_MESON
>>>>    	select COMMON_CLK_AXG
>>>>    	select COMMON_CLK_G12A
>>>>    	select MESON_IRQ_GPIO
>>>> +	select COMMON_CLK_A1
>>>
>>> This need to be separate patch addressed to Kevin once your driver is merged
>>>
>> ok, I will remove it in PATCH V2. And send it again after the driver is
>> merged.
>>>>    	help
>>>>    	  This enables support for the arm64 based Amlogic SoCs
>>>>    	  such as the s905, S905X/D, S912, A113X/D or S905X/D2
>>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>>> index dabeb43..e6cb4c3 100644
>>>> --- a/drivers/clk/meson/Kconfig
>>>> +++ b/drivers/clk/meson/Kconfig
>>>> @@ -107,3 +107,13 @@ config COMMON_CLK_G12A
>>>>    	help
>>>>    	  Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
>>>>    	  devices, aka g12a. Say Y if you want peripherals to work.
>>>> +
>>>> +config COMMON_CLK_A1
>>>> +	bool
>>>> +	depends on ARCH_MESON
>>>> +	select COMMON_CLK_MESON_REGMAP
>>>> +	select COMMON_CLK_MESON_DUALDIV
>>>> +	select COMMON_CLK_MESON_PLL
>>>> +	help
>>>> +	  Support for the clock controller on Amlogic A113L device,
>>>> +	  aka a1. Say Y if you want peripherals to work.
>>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>>> index 3939f21..6be3a8f 100644
>>>> --- a/drivers/clk/meson/Makefile
>>>> +++ b/drivers/clk/meson/Makefile
>>>> @@ -19,3 +19,4 @@ obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>>>>    obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>>>>    obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
>>>>    obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>>>> +obj-$(CONFIG_COMMON_CLK_A1) += a1.o
>>>> diff --git a/drivers/clk/meson/a1.c b/drivers/clk/meson/a1.c
>>>> new file mode 100644
>>>> index 0000000..26edae0f
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/a1.c
>>>> @@ -0,0 +1,2617 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>> +/*
>>>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/clk-provider.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/of_address.h>
>>>> +#include "clk-mpll.h"
>>>> +#include "clk-pll.h"
>>>> +#include "clk-regmap.h"
>>>> +#include "vid-pll-div.h"
>>>> +#include "clk-dualdiv.h"
>>>> +#include "meson-eeclk.h"
>>>> +#include "a1.h"
>>>> +
>>>> +/* PLLs clock in gates, its parent is xtal */
>>>> +static struct clk_regmap a1_xtal_clktree = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_OSCIN_CTRL,
>>>> +		.bit_idx = 0,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "xtal_clktree",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IS_CRITICAL,
>>>
>>> Is CCF even expected to touch this ever ? what about RO ops ?
>>> Please review your other clocks with this in mind
>>>
>> the clock should not be changed at runtime.clk_regmap_gate_ro_ops
>> is a good idea. Set RO ops and remove the CLK_IS_CRITICAL flag.
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_xtal_fixpll = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_OSCIN_CTRL,
>>>> +		.bit_idx = 1,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "xtal_fixpll",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_xtal_usb_phy = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_OSCIN_CTRL,
>>>> +		.bit_idx = 2,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "xtal_usb_phy",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IS_CRITICAL,
>>>
>>> How is an USB clock critical to the system ?
>>> Please review your other clocks with comment in mind ...
>> the usb clock does not affect the system,
>> remove the CLK_IS_CRITICAL flag
>>>
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_xtal_usb_ctrl = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_OSCIN_CTRL,
>>>> +		.bit_idx = 3,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "xtal_usb_ctrl",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>> the usb clock does not affect the system,
>> remove the CLK_IS_CRITICAL flag
>>>> +
>>>> +static struct clk_regmap a1_xtal_hifipll = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_OSCIN_CTRL,
>>>> +		.bit_idx = 4,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "xtal_hifipll",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>> CLK_IS_CRITICAL is need to lock hifi pll.
> 
> That's not how CCF works, this falg is not ok here.
> CCF will enable this clock before calling enable on your hifi pll
> 
ok， I will remove it.
>>>> +
>>>> +static struct clk_regmap a1_xtal_syspll = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_OSCIN_CTRL,
>>>> +		.bit_idx = 5,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "xtal_syspll",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>>>> +
>> when CPU clock is at fixed clock, sys pll
>> will be disabled, xtal_syspll will be disabled too.
>> when setting sys pll, call clk_set_rate to lock
>> sys pll, add RO ops to avoid disabling the clock
> 
> Again not Ok.
> If you mechanism to lock the PLL is properly implemented in the enable
> callback of the sys pll, still kind of work around are not needed
> 
> This has worked on the pll we had so far.
> 
ok, I will remove it.
>>
>>>> +static struct clk_regmap a1_xtal_dds = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_OSCIN_CTRL,
>>>> +		.bit_idx = 6,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "xtal_dds",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>> CLK_IS_CRITICAL is need to lock dds
>>>> +
>>>> +/* fixed pll = 1536M
>>>> + *
>>>> + * fixed pll ----- fclk_div2 = 768M
>>>> + *           |
>>>> + *           ----- fclk_div3 = 512M
>>>> + *           |
>>>> + *           ----- fclk_div5 = 307.2M
>>>> + *           |
>>>> + *           ----- fclk_div7 = 219.4M
>>>> + */
>>>
>>> The framework will make those calculation ... you can remove this
>>>
>> ok, I will remote the comment.
>>>> +static struct clk_regmap a1_fixed_pll_dco = {
>>>> +	.data = &(struct meson_clk_pll_data){
>>>> +		.en = {
>>>> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
>>>> +			.shift   = 28,
>>>> +			.width   = 1,
>>>> +		},
>>>> +		.m = {
>>>> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
>>>> +			.shift   = 0,
>>>> +			.width   = 8,
>>>> +		},
>>>> +		.n = {
>>>> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
>>>> +			.shift   = 10,
>>>> +			.width   = 5,
>>>> +		},
>>>> +		.frac = {
>>>> +			.reg_off = ANACTRL_FIXPLL_CTRL1,
>>>> +			.shift   = 0,
>>>> +			.width   = 19,
>>>> +		},
>>>> +		.l = {
>>>> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
>>>> +			.shift   = 31,
>>>> +			.width   = 1,
>>>> +		},
>>>> +		.rst = {
>>>> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
>>>> +			.shift   = 29,
>>>> +			.width   = 1,
>>>> +		},
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fixed_pll_dco",
>>>> +		.ops = &meson_clk_pll_ro_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_xtal_fixpll.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_fixed_pll = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = ANACTRL_FIXPLL_CTRL0,
>>>> +		.bit_idx = 20,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "fixed_pll",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fixed_pll_dco.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +static const struct pll_params_table a1_hifi_pll_params_table[] = {
>>>> +	PLL_PARAMS(128, 5), /* DCO = 614.4M */
>>>> +};
>>>> +
>>>> +static const struct reg_sequence a1_hifi_init_regs[] = {
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL1,	.def = 0x01800000 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL2,	.def = 0x00001100 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL3,	.def = 0x10022200 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL4,	.def = 0x00301000 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f19480 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f19480, .delay_us = 10 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL0,	.def = 0x15f11480, .delay_us = 40 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL2,	.def = 0x00001140 },
>>>> +	{ .reg = ANACTRL_HIFIPLL_CTRL2,	.def = 0x00001100 },
>>>> +};
>>>> +
>>>> +/*
>>>> + * The Meson A1 HIFI PLL is 614.4M, it requires
>>>> + * a strict register sequence to enable the PLL.
>>>> + * set meson_clk_pcie_pll_ops as its ops
>>>> + */
>>>
>>> Could you elaborate on this ? What need to be done to enable the clock ?
>>> Also the HIFI PLL used to be able to do a *LOT* of different rate which
>>> might be desirable for audio use case. Why is this one restricted to one
>>> particular rate ?
>>>
>> The audio working frequency are 44.1khz, 48khz and 192khz.
>>
>> 614.4M can meet the three frequency.
>>
>> after the hifi pll, there are two dividers in Audio clock.
>>
>> 614.4M/3200 = 192khz
>>
>> 614.4M/12800 = 48khz
>>
>> 614,4M/13932 = 44.0999khz
> 
> It does not really answer my question though.
> You are locking a use case here, which is 32 bit sample width
> 
> We have other constraint with the upstream audio driver, and we usually
> looking for base frequency that a multiple of 768 (24*32).
> 
> If you need your PLL to be set to a particular rate for a use case, the
> correct way is "assigned-rate" in DT
> 
> so the question still stands, the HIFI pll before was pretty easy to set
> at a wide variety of rate (same as GP0) ... is it not the case anymore ?
> If yes, could you decribe the constraints.
> 
> All this took us a long time to figure out on our own, which is why I'd
> prefer to get the proper constraints in from the beginning this time
> 
ok, I will verify it and  describe the constraints about it
> 
>>
>>>> +static struct clk_regmap a1_hifi_pll = {
>>>> +	.data = &(struct meson_clk_pll_data){
>>>> +		.en = {
>>>> +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>>>> +			.shift   = 28,
>>>> +			.width   = 1,
>>>> +		},
>>>> +		.m = {
>>>> +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>>>> +			.shift   = 0,
>>>> +			.width   = 8,
>>>> +		},
>>>> +		.n = {
>>>> +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>>>> +			.shift   = 10,
>>>> +			.width   = 5,
>>>> +		},
>>>> +		.frac = {
>>>> +			.reg_off = ANACTRL_HIFIPLL_CTRL1,
>>>> +			.shift   = 0,
>>>> +			.width   = 19,
>>>> +		},
>>>> +		.l = {
>>>> +			.reg_off = ANACTRL_HIFIPLL_STS,
>>>> +			.shift   = 31,
>>>> +			.width   = 1,
>>>> +		},
>>>> +		.table = a1_hifi_pll_params_table,
>>>> +		.init_regs = a1_hifi_init_regs,
>>>> +		.init_count = ARRAY_SIZE(a1_hifi_init_regs),
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "hifi_pll",
>>>> +		.ops = &meson_clk_pcie_pll_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_xtal_hifipll.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_fixed_factor a1_fclk_div2_div = {
>>>> +	.mult = 1,
>>>> +	.div = 2,
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div2_div",
>>>> +		.ops = &clk_fixed_factor_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fixed_pll.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_fclk_div2 = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = ANACTRL_FIXPLL_CTRL0,
>>>> +		.bit_idx = 21,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div2",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fclk_div2_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		/*
>>>> +		 * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>>>> +		 * or its children clocks.
>>>> +		 */
>>>
>>> The meaning of this flag is already documented in clk-provider.h
>>> The reason why you need this flag is lot more interesting here ...
>>>
>>> Same below
>> ok, I will replace new comments here.
>>>
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_fixed_factor a1_fclk_div3_div = {
>>>> +	.mult = 1,
>>>> +	.div = 3,
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div3_div",
>>>> +		.ops = &clk_fixed_factor_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fixed_pll.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_fclk_div3 = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = ANACTRL_FIXPLL_CTRL0,
>>>> +		.bit_idx = 22,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div3",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fclk_div3_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		/*
>>>> +		 * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>>>> +		 * its children clocks.
>>>> +		 */
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_fixed_factor a1_fclk_div5_div = {
>>>> +	.mult = 1,
>>>> +	.div = 5,
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div5_div",
>>>> +		.ops = &clk_fixed_factor_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fixed_pll.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_fclk_div5 = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = ANACTRL_FIXPLL_CTRL0,
>>>> +		.bit_idx = 23,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div5",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fclk_div5_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		/*
>>>> +		 * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>>>> +		 * its children clocks.
>>>> +		 */
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_fixed_factor a1_fclk_div7_div = {
>>>> +	.mult = 1,
>>>> +	.div = 7,
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div7_div",
>>>> +		.ops = &clk_fixed_factor_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fixed_pll.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_fclk_div7 = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = ANACTRL_FIXPLL_CTRL0,
>>>> +		.bit_idx = 23,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div7",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fclk_div7_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		/*
>>>> +		 * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>>>> +		 * or its children clock.
>>>> +		 */
>>>> +		.flags = CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>>>> +
>>>> +/* sys clk = 64M */
>>>
>>> Again this comment does not help understanding the driver code, please
>>> remove
>>>
>> got it.
>>>> +static u32 mux_table_sys_ab_clk[] = { 0, 1, 2, 3};
>>>
>>> This table is useless
>>>
>>>> +static const struct clk_parent_data sys_ab_clk_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .hw = &a1_fclk_div2.hw },
>>>> +	{ .hw = &a1_fclk_div3.hw },
>>>> +	{ .hw = &a1_fclk_div5.hw },
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_sys_b_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = SYS_CLK_CTRL0,
>>>> +		.mask = 0x7,
>>>> +		.shift = 26,
>>>> +		.table = mux_table_sys_ab_clk,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "sys_b_sel",
>>>> +		.ops = &clk_regmap_mux_ro_ops,
>>>> +		.parent_data = sys_ab_clk_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(sys_ab_clk_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_sys_b_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = SYS_CLK_CTRL0,
>>>> +		.shift = 16,
>>>> +		.width = 10,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "sys_b_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_sys_b_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_sys_b = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_CLK_CTRL0,
>>>> +		.bit_idx = 29,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "sys_b",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_sys_b_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>>
>>> Why does this need to be critical ?
>>> The a1_sys_clk (leaf of the block) could possibly be critical (if you
>>> explain why) ... but the flag should not be needed in the middle of the
>>> block
>>>
>> ok, the a1_sys_clk should not be changed at runtime, it is the same with
>> the clk81 clock in G12A. Just use another name in A1.
> 
> This does not justify the clock to be critical.
> 
> #1 Why can't it be changed ?
> #2 CLK_IS_CRITICAL does not say that clock can't change, it says it
>     can't gate ... two different things
> 
> 
DDR clock's parent clcok is fclk_div2, It can not be gated.I will add
add commits.
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_sys_a_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = SYS_CLK_CTRL0,
>>>> +		.mask = 0x7,
>>>> +		.shift = 10,
>>>> +		.table = mux_table_sys_ab_clk,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "sys_a_sel",
>>>> +		.ops = &clk_regmap_mux_ro_ops,
>>>> +		.parent_data = sys_ab_clk_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(sys_ab_clk_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_sys_a_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = SYS_CLK_CTRL0,
>>>> +		.shift = 0,
>>>> +		.width = 10,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "sys_a_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_sys_a_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_sys_a = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SYS_CLK_CTRL0,
>>>> +		.bit_idx = 13,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "sys_a",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_sys_a_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_sys_clk = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = SYS_CLK_CTRL0,
>>>> +		.mask = 0x1,
>>>> +		.shift = 31,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "sys_clk",
>>>> +		.ops = &clk_regmap_mux_ro_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_sys_a.hw, &a1_sys_b.hw,
>>>> +		},
>>>> +		.num_parents = 2,
>>>
>>> SET_RATE_PARENT ?
>>>
>> the ops is RO, Maybe no flag is better.
> 
> Is RO for now but setting the rate propagation properly fromt the
> beginning is preferred.
> 
> In general, you should have SET_RATE_PARENT everywhere unless there is a
> reason to stop the rate propagation.
> 
OK, keep SET_RATE_PARENT here
>>>> +	},
>>>> +};
>>>> +
>>>> +/* rtc 32k clock in */
>>>> +static struct clk_regmap a1_rtc_32k_clkin = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = RTC_BY_OSCIN_CTRL0,
>>>> +		.bit_idx = 31,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "rtc_32k_clkin",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static const struct meson_clk_dualdiv_param a1_32k_div_table[] = {
>>>> +	{
>>>> +		.dual		= 1,
>>>> +		.n1		= 733,
>>>> +		.m1		= 8,
>>>> +		.n2		= 732,
>>>> +		.m2		= 11,
>>>> +	}, {}
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_rtc_32k_div = {
>>>> +	.data = &(struct meson_clk_dualdiv_data){
>>>> +		.n1 = {
>>>> +			.reg_off = RTC_BY_OSCIN_CTRL0,
>>>> +			.shift   = 0,
>>>> +			.width   = 12,
>>>> +		},
>>>> +		.n2 = {
>>>> +			.reg_off = RTC_BY_OSCIN_CTRL0,
>>>> +			.shift   = 12,
>>>> +			.width   = 12,
>>>> +		},
>>>> +		.m1 = {
>>>> +			.reg_off = RTC_BY_OSCIN_CTRL1,
>>>> +			.shift   = 0,
>>>> +			.width   = 12,
>>>> +		},
>>>> +		.m2 = {
>>>> +			.reg_off = RTC_BY_OSCIN_CTRL1,
>>>> +			.shift   = 12,
>>>> +			.width   = 12,
>>>> +		},
>>>> +		.dual = {
>>>> +			.reg_off = RTC_BY_OSCIN_CTRL0,
>>>> +			.shift   = 28,
>>>> +			.width   = 1,
>>>> +		},
>>>> +		.table = a1_32k_div_table,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "rtc_32k_div",
>>>> +		.ops = &meson_clk_dualdiv_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_rtc_32k_clkin.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_rtc_32k_xtal = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = RTC_BY_OSCIN_CTRL1,
>>>> +		.bit_idx = 24,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "rtc_32k_xtal",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_rtc_32k_clkin.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static u32 rtc_32k_sel[] = { 0, 1 };
>>>
>>> useless
>>>
>> ok, I will delete the table.
>>>> +
>>>> +static struct clk_regmap a1_rtc_32k_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data) {
>>>> +		.offset = RTC_CTRL,
>>>> +		.mask = 0x3,
>>>> +		.shift = 0,
>>>> +		.table = rtc_32k_sel,
>>>> +		.flags = CLK_MUX_ROUND_CLOSEST,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "rtc_32k_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_rtc_32k_xtal.hw,
>>>> +			&a1_rtc_32k_div.hw,
>>>> +		},
>>>> +		.num_parents = 2,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +struct clk_regmap a1_rtc_clk = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = RTC_BY_OSCIN_CTRL0,
>>>> +		.bit_idx = 30,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "rtc_clk",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_rtc_32k_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +/* dsp a clk */
>>>> +static u32 mux_table_dsp_ab[] = { 0, 1, 2, 3, 4, 7 };
>>>> +static const struct clk_parent_data dsp_ab_clk_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .hw = &a1_fclk_div2.hw },
>>>> +	{ .hw = &a1_fclk_div3.hw },
>>>> +	{ .hw = &a1_fclk_div5.hw },
>>>> +	{ .hw = &a1_hifi_pll.hw },
>>>> +	{ .hw = &a1_rtc_clk.hw },
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_a_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DSPA_CLK_CTRL0,
>>>> +		.mask = 0x7,
>>>> +		.shift = 10,
>>>> +		.table = mux_table_dsp_ab,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspa_a_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = dsp_ab_clk_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_a_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = DSPA_CLK_CTRL0,
>>>> +		.shift = 0,
>>>> +		.width = 10,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspa_a_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspa_a_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_a = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPA_CLK_CTRL0,
>>>> +		.bit_idx = 13,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspa_a",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspa_a_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>
>>> This CLK_IGNORE_UNUSED, if justified, needs to be explained
>>>
>> ok, DSP cowork withe ARM in A1, dsp clock shoud not be changed at
>> runtime.
> 
> #1: UNUSED only guarantee that the clock won't be gated in the late init
>   if. No guarantee whatsoever after that.
> #2: If you have a DSP that need a clock active at particular rate, write
>   a driver for it and use the clock API. Don't throw your use case
>   constraints in the tree def.
> 
> Please review all occurrences of CRITICAL, RO, IGNORE_UNUSED before
> sending a v2.
> 
Yes, Dsp related driver should enable the clock itsself, remove 
CLK_IGNORE_UNUSED
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_b_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DSPA_CLK_CTRL0,
>>>> +		.mask = 0x7,
>>>> +		.shift = 26,
>>>> +		.table = mux_table_dsp_ab,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspa_b_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = dsp_ab_clk_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_b_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = DSPA_CLK_CTRL0,
>>>> +		.shift = 16,
>>>> +		.width = 10,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspa_b_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspa_b_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_b = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPA_CLK_CTRL0,
>>>> +		.bit_idx = 29,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspa_b",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspa_b_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DSPA_CLK_CTRL0,
>>>> +		.mask = 0x1,
>>>> +		.shift = 15,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspa_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = (const struct clk_parent_data []) {
>>>> +			{ .hw = &a1_dspa_a.hw },
>>>> +			{ .hw = &a1_dspa_b.hw },
>>>> +		},
>>>> +		.num_parents = 2,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_en_dspa = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPA_CLK_EN,
>>>> +		.bit_idx = 1,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspa_en_dspa",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspa_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspa_en_nic = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPA_CLK_EN,
>>>> +		.bit_idx = 0,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspa_en_nic",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspa_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +/* dsp b clk */
>>>> +static struct clk_regmap a1_dspb_a_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DSPB_CLK_CTRL0,
>>>> +		.mask = 0x7,
>>>> +		.shift = 10,
>>>> +		.table = mux_table_dsp_ab,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspb_a_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = dsp_ab_clk_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_a_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = DSPB_CLK_CTRL0,
>>>> +		.shift = 0,
>>>> +		.width = 10,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspb_a_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspb_a_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_a = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPB_CLK_CTRL0,
>>>> +		.bit_idx = 13,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspb_a",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspb_a_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_b_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DSPB_CLK_CTRL0,
>>>> +		.mask = 0x7,
>>>> +		.shift = 26,
>>>> +		.table = mux_table_dsp_ab,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspb_b_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = dsp_ab_clk_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_b_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = DSPB_CLK_CTRL0,
>>>> +		.shift = 16,
>>>> +		.width = 10,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspb_b_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspb_b_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_b = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPB_CLK_CTRL0,
>>>> +		.bit_idx = 29,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspb_b",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspb_b_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DSPB_CLK_CTRL0,
>>>> +		.mask = 0x1,
>>>> +		.shift = 15,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dspb_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspb_a.hw, &a1_dspb_b.hw,
>>>> +		},
>>>> +		.num_parents = 2,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_en_dspb = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPB_CLK_EN,
>>>> +		.bit_idx = 1,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspb_en_dspb",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspb_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dspb_en_nic = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DSPB_CLK_EN,
>>>> +		.bit_idx = 0,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dspb_en_nic",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dspb_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +/* 12M/24M clock */
>>>> +static struct clk_regmap a1_24m = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = CLK12_24_CTRL,
>>>> +		.bit_idx = 11,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "24m",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>> +			.fw_name = "xtal",
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_fixed_factor a1_24m_div2 = {
>>>> +	.mult = 1,
>>>> +	.div = 2,
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "24m_div2",
>>>> +		.ops = &clk_fixed_factor_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_24m.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_12m = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = CLK12_24_CTRL,
>>>> +		.bit_idx = 10,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "12m",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_24m_div2.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_fclk_div2_divn_pre = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = CLK12_24_CTRL,
>>>> +		.shift = 0,
>>>> +		.width = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div2_divn_pre",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fclk_div2.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_fclk_div2_divn = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = CLK12_24_CTRL,
>>>> +		.bit_idx = 12,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div2_divn",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_fclk_div2_divn_pre.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +/* gen clk */
>>>
>>> Either put something helpful or don't put anything
>>> For example explaining why there is a hole in the table bellow and what
>>> is parent 4 (if any) would be helpful.
>> ok, parent 2 is sys_pll_div16, sys pll and related clock are not
>> completeed, parent 4 is one of clock measurement source, it relies on
>> the clock measurement register configuration.
>> gen clock is a debug and monitor clock.
>>>
>>>> +static u32 gen_clk_table[] = { 0, 1, 3, 5, 6, 7, 8 };
>>>> +static const struct clk_parent_data gen_clk_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .hw = &a1_rtc_clk.hw },
>>>> +	{ .hw = &a1_hifi_pll.hw },
>>>> +	{ .hw = &a1_fclk_div2.hw },
>>>> +	{ .hw = &a1_fclk_div3.hw },
>>>> +	{ .hw = &a1_fclk_div5.hw },
>>>> +	{ .hw = &a1_fclk_div7.hw },
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_gen_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = GEN_CLK_CTRL,
>>>> +		.mask = 0xf,
>>>> +		.shift = 12,
>>>> +		.table = gen_clk_table,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "gen_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = gen_clk_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(gen_clk_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_gen_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = GEN_CLK_CTRL,
>>>> +		.shift = 0,
>>>> +		.width = 11,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "gen_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_gen_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_gen = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = GEN_CLK_CTRL,
>>>> +		.bit_idx = 11,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "gen",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_gen_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_saradc_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = SAR_ADC_CLK_CTRL,
>>>> +		.mask = 0x1,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "saradc_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = (const struct clk_parent_data []) {
>>>> +			{ .fw_name = "xtal", },
>>>> +			{ .hw = &a1_sys_clk.hw, },
>>>> +		},
>>>> +		.num_parents = 2,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_saradc_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = SAR_ADC_CLK_CTRL,
>>>> +		.shift = 0,
>>>> +		.width = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "saradc_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_saradc_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_saradc_clk = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = SAR_ADC_CLK_CTRL,
>>>> +		.bit_idx = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "saradc_clk",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_saradc_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +/* pwm a/b/c/d parent data */
>>>> +static const struct clk_parent_data pwm_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .hw = &a1_sys_clk.hw },
>>>> +};
>>>> +
>>>> +/*
>>>> + * add CLK_IGNORE_UNUSED flag for pwm controller GATE
>>>> + * clk core will disable unused clock, it may disable
>>>> + * vddcore voltage which contrlled by one pwm in bl21.
>>>                                ^
>>>                                typo
>>>
>>>> + * add the flag to avoid changing cpu voltage.
>>>
>>> 80 char per line is ok
>>> This comment would be better placed next to the corresponding flag
>>>
>>> Also you could explain that flag could be removed when CCF has some
>>> hand-off mechanism (similar comments you can copy in other driver)
>>>
>> ok,  contrlled --> controlled
>> I will move the comment next to the flag
>> I have gotten the similar comments in g12a.c
>>>> + */
>>>> +/* pwm a clk */
>>>> +static struct clk_regmap a1_pwm_a_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = PWM_CLK_AB_CTRL,
>>>> +		.mask = 0x1,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "pwm_a_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = pwm_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(pwm_parent_data),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_pwm_a_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = PWM_CLK_AB_CTRL,
>>>> +		.shift = 0,
>>>> +		.width = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "pwm_a_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_pwm_a_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_pwm_a = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = PWM_CLK_AB_CTRL,
>>>> +		.bit_idx = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "pwm_a",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_pwm_a_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>
>> [...]
>>
>>>> +/* usb bus clk */
>>>> +static const struct clk_parent_data usb_bus_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .hw = &a1_sys_clk.hw },
>>>> +	{ .hw = &a1_fclk_div3.hw },
>>>> +	{ .hw = &a1_fclk_div5.hw },
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_usb_bus_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = USB_BUSCLK_CTRL,
>>>> +		.mask = 0x3,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "usb_bus_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = usb_bus_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(usb_bus_parent_data),
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>
>>> Again, every unused flag need to be properly explained
>>>
>> ok, I will confirm this flag
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_usb_bus_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = USB_BUSCLK_CTRL,
>>>> +		.shift = 0,
>>>> +		.width = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "usb_bus_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_usb_bus_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_usb_bus = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = USB_BUSCLK_CTRL,
>>>> +		.bit_idx = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "usb_bus",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_usb_bus_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>
>> [...]
>>
>>>> +
>>>> +/* dmc clk */
>>>> +static const struct clk_hw *dmc_parent_hws[] = {
>>>> +	&a1_fclk_div2.hw,
>>>> +	&a1_fclk_div3.hw,
>>>> +	&a1_fclk_div5.hw,
>>>> +	&a1_hifi_pll.hw,
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dmc_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DMC_CLK_CTRL,
>>>> +		.mask = 0x3,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dmc_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_hws = dmc_parent_hws,
>>>> +		.num_parents = ARRAY_SIZE(dmc_parent_hws),
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dmc_div = {
>>>> +	.data = &(struct clk_regmap_div_data){
>>>> +		.offset = DMC_CLK_CTRL,
>>>> +		.shift = 0,
>>>> +		.width = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dmc_div",
>>>> +		.ops = &clk_regmap_divider_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dmc_sel.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dmc_sel2 = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = DMC_CLK_CTRL,
>>>> +		.mask = 0x1,
>>>> +		.shift = 15,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "dmc_sel2",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = (const struct clk_parent_data []) {
>>>> +			{ .hw = &a1_dmc_div.hw },
>>>> +			{ .fw_name = "xtal", },
>>>> +		},
>>>> +		.num_parents = 2,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct clk_regmap a1_dmc = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = DMC_CLK_CTRL,
>>>> +		.bit_idx = 8,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "dmc",
>>>> +		.ops = &clk_regmap_gate_ops,
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&a1_dmc_sel2.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		/*
>>>> +		 * add CLK_IGNORE_UNUSED to avoid hangup
>>>> +		 * DDR clock should not change at runtime
>>>> +		 */
>>>
>>> CRITICAL ? RO ops maybe ?
>> RO ops is better
>>>
>>>> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>> +	},
>>>> +};
>>>> +
>>
>> [...]
>>
>>>> +
>>>> +#define MESON_GATE(_name, _reg, _bit) \
>>>> +	MESON_PCLK(_name, _reg, _bit, &a1_sys_clk.hw)
>>>> +
>>>> +/* CLKTREE_SYS_CLK_EN0 */
>>>
>>> Full caps is not ok ... and once again, try to add comment which helps
>> ok, I will add some useful comment.
>>>
>>>> +static MESON_GATE(a1_clk_tree,		SYS_CLK_EN0,	0);
>>>> +static MESON_GATE(a1_reset_ctrl,	SYS_CLK_EN0,	1);
>>>> +static MESON_GATE(a1_analog_ctrl,	SYS_CLK_EN0,	2);
>>>> +static MESON_GATE(a1_pwr_ctrl,		SYS_CLK_EN0,	3);
>>>> +static MESON_GATE(a1_pad_ctrl,		SYS_CLK_EN0,	4);
>>>> +static MESON_GATE(a1_sys_ctrl,		SYS_CLK_EN0,	5);
>>>> +static MESON_GATE(a1_temp_sensor,	SYS_CLK_EN0,	6);
>>>> +static MESON_GATE(a1_am2axi_dev,	SYS_CLK_EN0,	7);
>>>> +static MESON_GATE(a1_spicc_b,		SYS_CLK_EN0,	8);
>>>> +static MESON_GATE(a1_spicc_a,		SYS_CLK_EN0,	9);
>>>> +static MESON_GATE(a1_clk_msr,		SYS_CLK_EN0,	10);
>>>> +static MESON_GATE(a1_audio,		SYS_CLK_EN0,	11);
>>>> +static MESON_GATE(a1_jtag_ctrl,		SYS_CLK_EN0,	12);
>>>> +static MESON_GATE(a1_saradc,		SYS_CLK_EN0,	13);
>>>> +static MESON_GATE(a1_pwm_ef,		SYS_CLK_EN0,	14);
>>>> +static MESON_GATE(a1_pwm_cd,		SYS_CLK_EN0,	15);
>>>> +static MESON_GATE(a1_pwm_ab,		SYS_CLK_EN0,	16);
>>>> +static MESON_GATE(a1_cec,		SYS_CLK_EN0,	17);
>>>> +static MESON_GATE(a1_i2c_s,		SYS_CLK_EN0,	18);
>>>> +static MESON_GATE(a1_ir_ctrl,		SYS_CLK_EN0,	19);
>>>> +static MESON_GATE(a1_i2c_m_d,		SYS_CLK_EN0,	20);
>>>> +static MESON_GATE(a1_i2c_m_c,		SYS_CLK_EN0,	21);
>>>> +static MESON_GATE(a1_i2c_m_b,		SYS_CLK_EN0,	22);
>>>> +static MESON_GATE(a1_i2c_m_a,		SYS_CLK_EN0,	23);
>>>> +static MESON_GATE(a1_acodec,		SYS_CLK_EN0,	24);
>>>> +static MESON_GATE(a1_otp,		SYS_CLK_EN0,	25);
>>>> +static MESON_GATE(a1_sd_emmc_a,		SYS_CLK_EN0,	26);
>>>> +static MESON_GATE(a1_usb_phy,		SYS_CLK_EN0,	27);
>>>> +static MESON_GATE(a1_usb_ctrl,		SYS_CLK_EN0,	28);
>>>> +static MESON_GATE(a1_sys_dspb,		SYS_CLK_EN0,	29);
>>>> +static MESON_GATE(a1_sys_dspa,		SYS_CLK_EN0,	30);
>>>> +static MESON_GATE(a1_dma,		SYS_CLK_EN0,	31);
>>>> +/* CLKTREE_SYS_CLK_EN1 */
>>>> +static MESON_GATE(a1_irq_ctrl,		SYS_CLK_EN1,	0);
>>>> +static MESON_GATE(a1_nic,		SYS_CLK_EN1,	1);
>>>> +static MESON_GATE(a1_gic,		SYS_CLK_EN1,	2);
>>>> +static MESON_GATE(a1_uart_c,		SYS_CLK_EN1,	3);
>>>> +static MESON_GATE(a1_uart_b,		SYS_CLK_EN1,	4);
>>>> +static MESON_GATE(a1_uart_a,		SYS_CLK_EN1,	5);
>>>> +static MESON_GATE(a1_sys_psram,		SYS_CLK_EN1,	6);
>>>> +static MESON_GATE(a1_rsa,		SYS_CLK_EN1,	8);
>>>> +static MESON_GATE(a1_coresight,		SYS_CLK_EN1,	9);
>>>> +/* CLKTREE_AXI_CLK_EN */
>>>> +static MESON_GATE(a1_am2axi_vad,	AXI_CLK_EN,	0);
>>>> +static MESON_GATE(a1_audio_vad,		AXI_CLK_EN,	1);
>>>> +static MESON_GATE(a1_axi_dmc,		AXI_CLK_EN,	3);
>>>> +static MESON_GATE(a1_axi_psram,		AXI_CLK_EN,	4);
>>>> +static MESON_GATE(a1_ramb,		AXI_CLK_EN,	5);
>>>> +static MESON_GATE(a1_rama,		AXI_CLK_EN,	6);
>>>> +static MESON_GATE(a1_axi_spifc,		AXI_CLK_EN,	7);
>>>> +static MESON_GATE(a1_axi_nic,		AXI_CLK_EN,	8);
>>>> +static MESON_GATE(a1_axi_dma,		AXI_CLK_EN,	9);
>>>> +static MESON_GATE(a1_cpu_ctrl,		AXI_CLK_EN,	10);
>>>> +static MESON_GATE(a1_rom,		AXI_CLK_EN,	11);
>>>> +static MESON_GATE(a1_prod_i2c,		AXI_CLK_EN,	12);
>>>> +
>>
>> [...]
>>
>>>> +
>>>> +/* Convenience table to populate regmap in .probe
>>>> + * Peripheral clock register base address is 0xfe000800
>>>
>>> this info will be in DT
>> I will remove the address commit
>>>
>>>> + */
>>
>> [...]
>>
>>>> +
>>>> +/*
>>>> + * cpu clock register base address is 0xfd000080
>>>> + */
>>>> +static struct clk_regmap *const a1_cpu_clk_regmaps[] = {
>>>> +	/* TODO */
>>>> +};
>>>> +
>>>> +/*
>>>> + * pll clock register base address is 0xfe007c00
>>>> + */
>>>> +static struct clk_regmap *const a1_pll_clk_regmaps[] = {
>>>> +	&a1_fixed_pll_dco,
>>>> +	&a1_fixed_pll,
>>>> +	&a1_hifi_pll,
>>>> +	&a1_fclk_div2,
>>>> +	&a1_fclk_div3,
>>>> +	&a1_fclk_div5,
>>>> +	&a1_fclk_div7,
>>>> +};
>>>> +
>>>> +static struct regmap_config clkc_regmap_config = {
>>>> +	.reg_bits       = 32,
>>>> +	.val_bits       = 32,
>>>> +	.reg_stride     = 4,
>>>> +};
>>>> +
>>>> +static struct regmap *a1_regmap_resource(struct device *dev, char *name)
>>>> +{
>>>> +	struct resource res;
>>>> +	void __iomem *base;
>>>> +	int i;
>>>> +	struct device_node *node = dev->of_node;
>>>> +
>>>> +	i = of_property_match_string(node, "reg-names", name);
>>>> +	if (of_address_to_resource(node, i, &res))
>>>> +		return ERR_PTR(-ENOENT);
>>>> +
>>>> +	base = devm_ioremap_resource(dev, &res);
>>>> +	if (IS_ERR(base))
>>>> +		return ERR_CAST(base);
>>>> +
>>>> +	clkc_regmap_config.max_register = resource_size(&res) - 4;
>>>> +	clkc_regmap_config.name = devm_kasprintf(dev, GFP_KERNEL,
>>>> +						 "%s-%s", node->name,
>>>> +						 name);
>>>> +	if (!clkc_regmap_config.name)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	return devm_regmap_init_mmio(dev, base, &clkc_regmap_config);
>>>> +}
>>>> +
>>>> +static int a1_clkc_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct regmap *peripheral_map, *pll_map, *cpu_map;
>>>> +	int ret, i;
>>>> +
>>>> +	/* Get regmap for different clock area */
>>>> +	peripheral_map = a1_regmap_resource(dev, "peripheral");
>>>> +	if (IS_ERR(peripheral_map)) {
>>>> +		dev_err(dev, "peripheral clk registers not found\n");
>>>> +		return PTR_ERR(peripheral_map);
>>>> +	}
>>>> +
>>>> +	pll_map = a1_regmap_resource(dev, "pll");
>>>> +	if (IS_ERR(pll_map)) {
>>>> +		dev_err(dev, "pll clk registers not found\n");
>>>> +		return PTR_ERR(pll_map);
>>>> +	}
>>>> +
>>>> +	cpu_map = a1_regmap_resource(dev, "cpu");
>>>> +	if (IS_ERR(cpu_map)) {
>>>> +		dev_err(dev, "cpu clk registers not found\n");
>>>> +		return PTR_ERR(cpu_map);
>>>> +	}
>>>> +
>>>> +	/* Populate regmap for the regmap backed clocks */
>>>> +	for (i = 0; i < ARRAY_SIZE(a1_clk_regmaps); i++)
>>>> +		a1_clk_regmaps[i]->map = peripheral_map;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(a1_cpu_clk_regmaps); i++)
>>>> +		a1_cpu_clk_regmaps[i]->map = cpu_map;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(a1_pll_clk_regmaps); i++)
>>>> +		a1_pll_clk_regmaps[i]->map = pll_map;
>>>
>>> Unless you have a rock solid reason why you absolutely need to make
>>> single controller out of this 3 regions, Nack.
>>>
>> As comment replying in
>> Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml, I will
>> implement three clock drivers.
>>>> +
>>>> +	for (i = 0; i < a1_hw_onecell_data.num; i++) {
>>>> +		/* array might be sparse */
>>>> +		if (!a1_hw_onecell_data.hws[i])
>>>> +			continue;
>>>> +
>>>> +		ret = devm_clk_hw_register(dev, a1_hw_onecell_data.hws[i]);
>>>> +		if (ret) {
>>>> +			dev_err(dev, "Clock registration failed\n");
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>>>> +					   &a1_hw_onecell_data);
>>>> +}
>>>> +
>>>> +static const struct of_device_id clkc_match_table[] = {
>>>> +	{ .compatible = "amlogic,a1-clkc" },
>>>> +	{ /* sentinel */ }
>>>> +};
>>>> +
>>>> +static struct platform_driver a1_driver = {
>>>> +	.probe		= a1_clkc_probe,
>>>> +	.driver		= {
>>>> +		.name	= "a1-clkc",
>>>> +		.of_match_table = clkc_match_table,
>>>> +	},
>>>> +};
>>>> +
>>>> +builtin_platform_driver(a1_driver);
>>>> diff --git a/drivers/clk/meson/a1.h b/drivers/clk/meson/a1.h
>>>> new file mode 100644
>>>> index 0000000..f2291c4
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/a1.h
>>>> @@ -0,0 +1,172 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>>>> +/*
>>>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>>>> + */
>>>> +
>>>> +#ifndef __A1_H
>>>> +#define __A1_H
>>>> +
>>>> +/*
>>>> + * peripheral clock controller register address
>>>> + * APB_BASE:  APB0_BASE_ADDR = 0xfe000800
>>>
>>> Again this info is DT and might become wrong if your controller is ever
>>> compatible with another SoC
>>>
>> I will remove the address comment, the right address is 0xfe000080.
>>>> + */
>>>> +#define SYS_OSCIN_CTRL			0x0
>>>> +#define RTC_BY_OSCIN_CTRL0		0x4
>>>> +#define RTC_BY_OSCIN_CTRL1		0x8
>>>> +#define RTC_CTRL			0xc
>>>> +#define SYS_CLK_CTRL0			0x10
>>>> +#define AXI_CLK_CTRL0			0x14
>>>> +#define SYS_CLK_EN0			0x1c
>>>> +#define SYS_CLK_EN1			0x20
>>>> +#define AXI_CLK_EN			0x24
>>>> +#define DSPA_CLK_EN			0x28
>>>> +#define DSPB_CLK_EN			0x2c
>>>> +#define DSPA_CLK_CTRL0			0x30
>>>> +#define DSPB_CLK_CTRL0			0x34
>>>> +#define CLK12_24_CTRL			0x38
>>>> +#define GEN_CLK_CTRL			0x3c
>>>> +#define TIMESTAMP_CTRL0			0x40
>>>> +#define TIMESTAMP_CTRL1			0x44
>>>> +#define TIMESTAMP_CTRL2			0x48
>>>> +#define TIMESTAMP_VAL0			0x4c
>>>> +#define TIMESTAMP_VAL1			0x50
>>>> +#define TIMEBASE_CTRL0			0x54
>>>> +#define TIMEBASE_CTRL1			0x58
>>>> +#define SAR_ADC_CLK_CTRL		0xc0
>>>> +#define PWM_CLK_AB_CTRL			0xc4
>>>> +#define PWM_CLK_CD_CTRL			0xc8
>>>> +#define PWM_CLK_EF_CTRL			0xcc
>>>> +#define SPICC_CLK_CTRL			0xd0
>>>> +#define TS_CLK_CTRL			0xd4
>>>> +#define SPIFC_CLK_CTRL			0xd8
>>>> +#define USB_BUSCLK_CTRL			0xdc
>>>> +#define SD_EMMC_CLK_CTRL		0xe0
>>>> +#define CECA_CLK_CTRL0			0xe4
>>>> +#define CECA_CLK_CTRL1			0xe8
>>>> +#define CECB_CLK_CTRL0			0xec
>>>> +#define CECB_CLK_CTRL1			0xf0
>>>> +#define PSRAM_CLK_CTRL			0xf4
>>>> +#define DMC_CLK_CTRL			0xf8
>>>> +#define FCLK_DIV1_SEL			0xfc
>>>> +#define TST_CTRL			0x100
>>>> +/*
>>>> + * For PLL register offset
>>>> + * APB_BASE:  APB0_BASE_ADDR = 0xfe007c00
>>>> + */
>>>> +#define ANACTRL_FIXPLL_CTRL0		0x80
>>>> +#define ANACTRL_FIXPLL_CTRL1		0x84
>>>> +#define ANACTRL_FIXPLL_CTRL2		0x88
>>>> +#define ANACTRL_FIXPLL_CTRL3		0x8c
>>>> +#define ANACTRL_FIXPLL_CTRL4		0x90
>>>> +#define ANACTRL_FIXPLL_STS		0x94
>>>> +#define ANACTRL_SYSPLL_CTRL0		0x100
>>>> +#define ANACTRL_SYSPLL_CTRL1		0x104
>>>> +#define ANACTRL_SYSPLL_CTRL2		0x108
>>>> +#define ANACTRL_SYSPLL_CTRL3		0x10c
>>>> +#define ANACTRL_SYSPLL_CTRL4		0x110
>>>> +#define ANACTRL_SYSPLL_STS		0x114
>>>> +#define ANACTRL_HIFIPLL_CTRL0		0x140
>>>> +#define ANACTRL_HIFIPLL_CTRL1		0x144
>>>> +#define ANACTRL_HIFIPLL_CTRL2		0x148
>>>> +#define ANACTRL_HIFIPLL_CTRL3		0x14c
>>>> +#define ANACTRL_HIFIPLL_CTRL4		0x150
>>>> +#define ANACTRL_HIFIPLL_STS		0x154
>>>> +#define ANACTRL_AUDDDS_CTRL0		0x180
>>>> +#define ANACTRL_AUDDDS_CTRL1		0x184
>>>> +#define ANACTRL_AUDDDS_CTRL2		0x188
>>>> +#define ANACTRL_AUDDDS_CTRL3		0x18c
>>>> +#define ANACTRL_AUDDDS_CTRL4		0x190
>>>> +#define ANACTRL_AUDDDS_STS		0x194
>>>> +#define ANACTRL_MISCTOP_CTRL0		0x1c0
>>>> +#define ANACTRL_POR_CNTL		0x208
>>>> +/*
>>>> + * CPU clok register offset
>>>> + * APB_BASE:  APB1_BASE_ADDR = 0xfd000080
>>>> + */
>>>> +#define CPUCTRL_CLK_CTRL0		0x00
>>>> +#define CPUCTRL_CLK_CTRL1		0x04
>>>> +#define CPUCTRL_CLK_CTRL2		0x08
>>>> +#define CPUCTRL_CLK_CTRL3		0x0c
>>>> +#define CPUCTRL_CLK_CTRL4		0x10
>>>> +#define CPUCTRL_CLK_CTRL5		0x14
>>>> +#define CPUCTRL_CLK_CTRL6		0x18
>>>> +#define CPUCTRL_CLK_CTRL7		0x1c
>>>> +
>>>> +#define CLKID_FIXED_PLL_DCO		0
>>>> +#define CLKID_SYS_A_SEL			10
>>>> +#define CLKID_SYS_A_DIV			11
>>>> +#define CLKID_SYS_A			12
>>>> +#define CLKID_SYS_B_SEL			13
>>>> +#define CLKID_SYS_B_DIV			14
>>>> +#define CLKID_SYS_B			15
>>>> +#define CLKID_XTAL_CLKTREE		18
>>>> +#define CLKID_XTAL_FIXPLL		19
>>>> +#define CLKID_XTAL_USB_PHY		20
>>>> +#define CLKID_XTAL_USB_CTRL		21
>>>> +#define CLKID_XTAL_HIFIPLL		22
>>>> +#define CLKID_XTAL_SYSPLL		23
>>>> +#define CLKID_XTAL_DDS			24
>>>> +#define CLKID_DSPA_A_SEL		78
>>>> +#define CLKID_DSPA_A_DIV		79
>>>> +#define CLKID_DSPA_A			80
>>>> +#define CLKID_DSPA_B_SEL		81
>>>> +#define CLKID_DSPA_B_DIV		82
>>>> +#define CLKID_DSPA_B			83
>>>> +#define CLKID_DSPB_A_SEL		85
>>>> +#define CLKID_DSPB_A_DIV		86
>>>> +#define CLKID_DSPB_A			87
>>>> +#define CLKID_DSPB_B_SEL		88
>>>> +#define CLKID_DSPB_B_DIV		89
>>>> +#define CLKID_DSPB_B			90
>>>> +#define CLKID_RTC_32K_CLKIN		96
>>>> +#define CLKID_RTC_32K_DIV		97
>>>> +#define CLKID_RTC_32K_XTAL		98
>>>> +#define CLKID_RTC_32K_SEL		99
>>>> +#define CLKID_CECB_32K_CLKIN		106
>>>> +#define CLKID_CECB_32K_DIV		107
>>>> +#define CLKID_CECB_32K_SEL_PRE		108
>>>> +#define CLKID_CECB_32K_SEL		109
>>>> +#define CLKID_CECA_32K_CLKIN		101
>>>> +#define CLKID_CECA_32K_DIV		102
>>>> +#define CLKID_CECA_32K_SEL_PRE		103
>>>> +#define CLKID_CECA_32K_SEL		104
>>>> +#define CLKID_DIV2_PRE			113
>>>> +#define CLKID_24M_DIV2			115
>>>> +#define CLKID_GEN_SEL			116
>>>> +#define CLKID_GEN_DIV			117
>>>> +#define CLKID_SARADC_DIV		120
>>>> +#define CLKID_PWM_A_SEL			122
>>>> +#define CLKID_PWM_A_DIV			123
>>>> +#define CLKID_PWM_B_SEL			125
>>>> +#define CLKID_PWM_B_DIV			126
>>>> +#define CLKID_PWM_C_SEL			128
>>>> +#define CLKID_PWM_C_DIV			129
>>>> +#define CLKID_PWM_D_SEL			131
>>>> +#define CLKID_PWM_D_DIV			132
>>>> +#define CLKID_PWM_E_SEL			134
>>>> +#define CLKID_PWM_E_DIV			135
>>>> +#define CLKID_PWM_F_SEL			137
>>>> +#define CLKID_PWM_F_DIV			138
>>>> +#define CLKID_SPICC_SEL			140
>>>> +#define CLKID_SPICC_DIV			141
>>>> +#define CLKID_SPICC_SEL2		142
>>>> +#define CLKID_TS_DIV			144
>>>> +#define CLKID_SPIFC_SEL			146
>>>> +#define CLKID_SPIFC_DIV			147
>>>> +#define CLKID_SPIFC_SEL2		148
>>>> +#define CLKID_USB_BUS_SEL		150
>>>> +#define CLKID_USB_BUS_DIV		151
>>>> +#define CLKID_SD_EMMC_SEL		153
>>>> +#define CLKID_SD_EMMC_DIV		154
>>>> +#define CLKID_SD_EMMC_SEL2		155
>>>> +#define CLKID_PSRAM_SEL			157
>>>> +#define CLKID_PSRAM_DIV			158
>>>> +#define CLKID_PSRAM_SEL2		159
>>>> +#define CLKID_DMC_SEL			161
>>>> +#define CLKID_DMC_DIV			162
>>>> +#define CLKID_DMC_SEL2			163
>>>> +#define NR_CLKS				165
>>>> +
>>>> +#include <dt-bindings/clock/a1-clkc.h>
>>>> +
>>>> +#endif /* __A1_H */
>>>> -- 
>>>> 1.9.1
>>>
>>> .
>>>
> 
> .
> 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic