lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1jimpd27cb.fsf@starbuckisacylon.baylibre.com>
Date:   Fri, 27 Sep 2019 15:32:04 +0200
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Jian Hu <jian.hu@...ogic.com>
Cc:     Jerome Brunet <jbrunet@...libre.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Jianxin Pan <jianxin.pan@...ogic.com>,
        Kevin Hilman <khilman@...libre.com>,
        Rob Herring <robh@...nel.org>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Qiufang Dai <qiufang.dai@...ogic.com>,
        linux-clk@...r.kernel.org, linux-amlogic@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] clk: meson: a1: add support for Amlogic A1 clock driver


On Fri 27 Sep 2019 at 11:52, Jian Hu <jian.hu@...ogic.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@...ogic.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@...ogic.com>
>>> Signed-off-by: Jianxin Pan <jianxin.pan@...ogic.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

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

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


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


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

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

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ