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

Jian Hu
2019-11-20T09:28:01Z

Hi, jerome

Is there any problem about fixed_pll_dco's parent_data?

Now both name and fw_name are described in parent_data.

More debug message is in the last email.

On 2019/11/13 22:21, Jian Hu wrote:
>
> On 2019/11/13 0:59, Jerome Brunet wrote:
>>
>> On Sat 09 Nov 2019 at 12:16, Jian Hu <jian.hu@amlogic.com> wrote:
>>
>>> Hi, Jerome
>>>
>>> Sorry for late rely
>>>
>>> On 2019/11/4 16:24, Jerome Brunet wrote:
>>>>
>>>> On Fri 25 Oct 2019 at 13:32, Jian Hu <jian.hu@amlogic.com> wrote:
>>>>
>>>>> Hi, Jerome
>>>>>
>>>>> Thanks for your review
>>>>>
>>>>> On 2019/10/21 19:41, Jerome Brunet wrote:
>>>>>>
>>>>>> On Fri 18 Oct 2019 at 09:14, Jian Hu <jian.hu@amlogic.com> wrote:
>>>>>>
>>>>>>> The Amlogic A1 clock includes three drivers:
>>>>>>> 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>
>>>>>>> ---
>>>>>>>     drivers/clk/meson/Kconfig  |   10 +
>>>>>>>     drivers/clk/meson/Makefile |    1 +
>>>>>>>     drivers/clk/meson/a1-pll.c |  345 +++++++
>>>>>>>     drivers/clk/meson/a1-pll.h |   56 ++
>>>>>>>     drivers/clk/meson/a1.c     | 2264
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     drivers/clk/meson/a1.h     |  120 +++
>>>>>>>     6 files changed, 2796 insertions(+)
>>>>>>>     create mode 100644 drivers/clk/meson/a1-pll.c
>>>>>>>     create mode 100644 drivers/clk/meson/a1-pll.h
>>>>>>>     create mode 100644 drivers/clk/meson/a1.c
>>>>>>>     create mode 100644 drivers/clk/meson/a1.h
>>>>>>
>>>>>> In the next version, one
>>>>> OK, I will send a1 peripheral and pll driver in two patch.
>>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>>>>>> index dabeb43..c2809b2 100644
>>>>>>> --- a/drivers/clk/meson/Kconfig
>>>>>>> +++ b/drivers/clk/meson/Kconfig
>>>>>>> @@ -93,6 +93,16 @@ config COMMON_CLK_AXG_AUDIO
>>>>>>>           Support for the audio clock controller on AmLogic A113D
>>>>>>> devices,
>>>>>>>           aka axg, Say Y if you want audio subsystem 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.
>>>>>>> +
>>>>>>>     config COMMON_CLK_G12A
>>>>>>>         bool
>>>>>>>         depends on ARCH_MESON
>>>>>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>>>>>> index 3939f21..28cbae1 100644
>>>>>>> --- a/drivers/clk/meson/Makefile
>>>>>>> +++ b/drivers/clk/meson/Makefile
>>>>>>> @@ -16,6 +16,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) +=
>>>>>>> vid-pll-div.o
>>>>>>>       obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
>>>>>>>     obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>>>>>>> +obj-$(CONFIG_COMMON_CLK_A1) += a1-pll.o a1.o
>>>>>>
>>>>>> So far, all the controller had there own option, I don't see why it
>>>>>> should be different here.
>>>>>>
>>>>> OK, I will add the other option CONFIG_COMMON_CLK_A1_PLL for pll
>>>>> driver
>>>>>>>     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
>>>>>>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..486d964
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/clk/meson/a1-pll.c
>>>>>>> @@ -0,0 +1,345 @@
>>>>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>>>>> +/*
>>>>>>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>>>>>>> + * Author: Jian Hu <jian.hu@amlogic.com>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <linux/platform_device.h>
>>>>>>
>>>>>> Hum ... looks like some things are missing here
>>>>>>
>>>>>> #include <linux/of_device.h>
>>>>>> #include <linux/clk-provider.h>
>>>>>>
>>>>>> ?
>>>>> #1
>>>>> There is <linux/clk-provider.h> in meson-eeclk.h file,
>>>>>
>>>>> and for A1 driver(a1.c/a1-pll.c) the head file is not requied.
>>>>>
>>>>> #2
>>>>> For A1 driver, the file "linux/of_device.h" is not required.
>>>>> It is required by meson-eeclk.c in fact.
>>>>
>>>> You are using what is provided by these headers directly in this file
>>>> If meson-eeclk ever changes, your driver breaks
>>>>
>>> OK, I will add the two header file.
>>>>>>
>>>>>>> +#include "clk-pll.h"
>>>>>>> +#include "meson-eeclk.h"
>>>>>>> +#include "a1-pll.h"
>>>>>>
>>>>>> Alphanumeric order please
>>>>>>
>>>>> OK, I will change it in the next version.
>>>>>
>>>>>>> +
>>>>>>> +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_data = &(const struct clk_parent_data){
>>>>>>> +            .fw_name = "xtal_fixpll",
>>>>>>> +            .name = "xtal_fixpll",
>>>>>>> +        },
>>>>>>> +        .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,
>>>>>>> +        /*
>>>>>>> +         * This clock is fclk_div2/3/4's parent,
>>>>>>> +         * However, fclk_div2/3/5 feeds AXI/APB/DDR.
>>>>>>
>>>>>> is it fclk_div2/3/4 or fclk_div2/3/5 ?
>>>>>>
>>>>>>> +         * It is required by the platform to operate correctly.
>>>>>>> +         * Until the following condition are met, we need this
>>>>>>> clock to
>>>>>>> +         * be marked as critical:
>>>>>>> +         * a) Mark the clock used by a firmware resource, if
>>>>>>> possible
>>>>>>> +         * b) CCF has a clock hand-off mechanism to make the
>>>>>>> sure the
>>>>>>> +         *    clock stays on until the proper driver comes along
>>>>>>> +         */
>>>>>>
>>>>>> Don't blindly copy/paste comments from other drivers. There is no
>>>>>> driver
>>>>>> for the devices you are mentionning so the end of the comment is
>>>>>> confusing. The 3 first lines were enough
>>>>>>
>>>>> OK, I will remove the confusing comments
>>>>>
>>>>>>> +        .flags = CLK_IS_CRITICAL,
>>>>>>
>>>>>> >From your comment, I understand that some child are critical, not
>>>>>> this
>>>>>> particular (or at least, not directly). So this can be removed AFAICT
>>>>>>
>>>>>> You should even need CLK_IGNORE_UNUSED for this one since the
>>>>>> clock will
>>>>>> already be enabled before the late_init() kicks in
>>>>>>
>>>>> OK, I will replace it as CLK_IGNORE_UNUSED.
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct pll_mult_range a1_hifi_pll_mult_range = {
>>>>>>> +    .min = 32,
>>>>>>> +    .max = 64,
>>>>>>> +};
>>>>>>> +
>>>>>>> +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 = 0x100a1100 },
>>>>>>> +    { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
>>>>>>> +    { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
>>>>>>> +    { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f18440, .delay_us
>>>>>>> = 10 },
>>>>>>> +    { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f18440, .delay_us
>>>>>>> = 40 },
>>>>>>> +    { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
>>>>>>> +    { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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,
>>>>>>> +        },
>>>>>>> +        .range = &a1_hifi_pll_mult_range,
>>>>>>> +        .init_regs = a1_hifi_init_regs,
>>>>>>> +        .init_count = ARRAY_SIZE(a1_hifi_init_regs),
>>>>>>> +        .strict_sequence = true,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "hifi_pll",
>>>>>>> +        .ops = &meson_clk_pll_ops,
>>>>>>> +        .parent_data = &(const struct clk_parent_data){
>>>>>>> +            .fw_name = "xtal_fixpll",
>>>>>>> +            .name = "xtal_fixpll",
>>>>>>
>>>>>> Both should provided when a controller transition from the old way of
>>>>>> describing parent to the new way. This is a new controller so it does
>>>>>> not apply.
>>>>> I do not understand why it does not apply, could you explain more?
>>>>
>>>> Your driver is new, it is not something old transitioning from global
>>>> name to clock in DT !
>>>>
>>>>>
>>>>> The xtal_fixpll clock is registered in another peripheral driver,
>>>>> If do not
>>>>> desribe the "name" member in parent_data, "fw_name" does not work
>>>>> because
>>>>> it has not been registered. the hifi_pll parent will be null in
>>>>> /sys/kernel/debug/clk/hifi_pll/clk_parent.
>>>>>
>>>>> HIFI PLL will be a orphan clock, but its parent should be xtal_fixpll.
>>>>
>>>> There will be an orphan yes, temporarily, until both controllers are
>>>> up.
>>>> Once both controller are up, the clock will be reparented if necessary.
>>>>
>>>>>
>>>>> So both of "fw_name" of "name" should be described.
>>>>
>>>> No
>>>>
>>>
>>> #1
>>>
>>> Here, I am still confused.
>>>  From the point of view of the phenomenon， the name in parent_data is
>>> required.
>>>
>>>
>>> HIFI PLL is described like this:
>>>
>>>     .parent_data = &(const struct clk_parent_data){
>>>         .fw_name = "xtal_hifipll",
>>>         .name = "xtal_hifipll"
>>>     }
>>>
>>> Fixed PLL is described like this:
>>>
>>>     .parent_data = &(const struct clk_parent_data){
>>>         .fw_name = "xtal_fixpll",
>>>     },
>>>
>>> After the system boot completely， run cat
>>> /sys/kernel/debug/clk/clk_summary, Here is the result:
>>>
>>> # cat /sys/kernel/debug/clk/clk_summary
>>>                   enable  prepare  protect                    duty
>>>     clock         count   count    count  rate accuracy phase cycle
>>> --------------------------------------------------------------------
>>>   xtal            5        5        0    24000000    0     0  50000
>>>      ts_div       0        0        0    24000000    0     0  50000
>>>         ts        0        0        0    24000000    0     0  50000
>>>      pwm_f_sel    0        0        0    24000000    0     0  50000
>>>         pwm_f_div 0        0        0    24000000    0     0  50000
>>>            pwm_f  0        0        0    24000000    0     0  50000
>>> ......
>>>      xtal_syspll  0        0        0    24000000    0     0  50000
>>>      xtal_hifipll 0        0        0    24000000    0     0  50000
>>>         hifi_pll  0        0        0  1536000000    0     0  50000
>>>      xtal_usb_ctrl 0        0        0    24000000   0     0  50000
>>>      xtal_usb_phy  0        0        0    24000000   0     0  50000
>>>      xtal_fixpll   0        0        0    24000000   0     0  50000
>>>      xtal_clktree  0        0        0    24000000   0     0  50000
>>>   fixed_pll_dco    1        1        0         0   0     0  50000
>>
>> This means that CCF what not able to resolve the parent.
>> Either:
>> * you've made a mistake somewhere
>> * There is bug in CCF when the parent device is not available at probe
>> time, the clock is not properly reparented
>>
>
> #1 Add DT describtion
>
> DT description:
>
> clkc_pll: pll-clock-controller {
>       compatible = "amlogic,a1-pll-clkc";
>       #clock-cells = <1>;
>       reg = <0 0x7c80 0 0x21c>;
>       clocks = <&clkc_periphs CLKID_XTAL_FIXPLL>,
>             <&clkc_periphs CLKID_XTAL_HIFIPLL>;
>       clock-names = "xtal_fixpll", "xtal_hifipll";
> };
>
> clkc_periphs: periphs-clock-controller {
>       compatible = "amlogic,a1-periphs-clkc";
>       #clock-cells = <1>;
>       reg = <0 0x800 0 0x104>;
>       clocks = <&clkc_pll CLKID_FCLK_DIV2>,
>                <&clkc_pll CLKID_FCLK_DIV3>,
>                <&clkc_pll CLKID_FCLK_DIV5>,
>                <&clkc_pll CLKID_FCLK_DIV7>,
>                <&clkc_pll CLKID_HIFI_PLL>,
>                <&xtal>;
>       clock-names = "fclk_div2", "fclk_div3", "fclk_div5",
>                     "fclk_div7", "hifi_pll", "xtal";
> };
>
> And PLL clock driver probe first.
>
> When register xtal_fixpll clock in periphs driver
>
> it will walk the list of orphan clocks, it will try to get the orphan
> parent.
>
> the code path is :
>
> struct clk_core *parent = __clk_init_parent(orphan)
>
>   -> clk_core_get_parent_by_index(core, index)
>
>    ->clk_core_fill_parent_index
>
>     ->clk_core_get
>
>      ->of_clk_get_hw_from_clkspec
>
> In of_clk_get_hw_from_clkspec function, it will get hw_clk from
> clk_provider, However the peripheral clock provider has not been
> added to "of_clk_providers" LIST, In other words, the peripheral clock
> provider does not exist at this time. (devm_of_clk_add_hw_provider has
> not run)
>
> So It will get parent failed from DT.
>
> even if name is added in parent_data, PTR_ERR(parent) is not equal
> -ENOENT, it will not run clk_core_lookup, the fixed_pll_dco is still a
> orphan clock.
>
> #2 Not add DT describtion
>
> the code path is :
>
> struct clk_core *parent = __clk_init_parent(orphan)
>
>   -> clk_core_get_parent_by_index(core, index)
>
>    ->clk_core_fill_parent_index
>
>     ->clk_core_get
>
>      ->clk_find_hw
>
> clk_find_hw is used when the clock is registered by clk_register_clkdev
>
> So it wlll get parented failed by clk_find_hw.
>
> When name is described in parent_data, the parent will be found by
>
> clk_core_lookup.
>
>
> In this scene，Only fw_name in parent_data, the orphan clock's parent
> can not be found, But add the legacy name, the parent will be found.
>
> #3
> When the provider has not been added to the CCF, And fw_name is alone in
> parent_data in a orphan clock , How to get the orphan clock'parent ?
> It seems not supported.
>
>> Either way, you'll have to debug a bit more
>>
>>>      fixed_pll     3        3        0         0   0     0  50000
>>>         fclk_div7_div 0     0        0         0   0     0  50000
>>> ....
>>>
>>> the hifi_pll's parent is xtal_hifi, And the hifi_pll default rate is
>>> right.
>>>
>>> but the fixed_pll_dco is a orphan clock, its parent is NULL.And its rate
>>> is zero.When the name in parent_data is added, its parent is
>>> xtal_fixpll.
>>>
>>> # cat /sys/kernel/debug/clk/fixed_pll_dco/clk_parent
>>> #
>>> # cat /sys/kernel/debug/clk/fixed_pll_dco/clk_rate
>>> 0
>>>
>>>
>>> #2
>>> In  ./drivers/clk/qcom/gcc-sm8150.c
>>> For some clocks, Both fw_name and name are described in parent_data
>>> struct.
>>
>> Those are being migrated to DT description which means that we don't
>> know if the DT will the correct description
>>
>> In this case the clock framework will first try DT and if DT does
>> provide this fw_name, it will fallback to the legacy name.
>>
>> This is not your case as this is a new platform for which we know the DT
>> name exist.
>>
> It will get parent failed from DT as the previous says.
>>>
>>>>>
>>>>>>
>>>>>> Same for the other occurences.
>>>>>>
>>>>>> Also, I think you meant xtal_hifipll
>>>>> Yes,I will correct it.
>>>>>>
>>>>>>> +        },
>>>>>>> +        .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,
>>>>>>> +        /*
>>>>>>> +         * This clock is used by DDR clock in BL2 firmware
>>>>>>> +         * and is required by the platform to operate correctly.
>>>>>>> +         * Until the following condition are met, we need this
>>>>>>> clock to
>>>>>>> +         * be marked as critical:
>>>>>>> +         * a) Mark the clock used by a firmware resource, if
>>>>>>> possible
>>>>>>> +         * b) CCF has a clock hand-off mechanism to make the
>>>>>>> sure the
>>>>>>> +         *    clock stays on until the proper driver comes along
>>>>>>> +         */
>>>>>>> +        .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,
>>>>>>> +        /*
>>>>>>> +         * This clock is used by APB bus which setted in Romcode
>>>>>>> +         * and is required by the platform to operate correctly.
>>>>>>> +         * Until the following condition are met, we need this
>>>>>>> clock to
>>>>>>> +         * be marked as critical:
>>>>>>> +         * a) Mark the clock used by a firmware resource, if
>>>>>>> possible
>>>>>>> +         * b) CCF has a clock hand-off mechanism to make the
>>>>>>> sure the
>>>>>>> +         *    clock stays on until the proper driver comes along
>>>>>>> +         */
>>>>>>> +        .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,
>>>>>>> +        /*
>>>>>>> +         * This clock is used by AXI bus which setted in Romcode
>>>>>>> +         * and is required by the platform to operate correctly.
>>>>>>> +         * Until the following condition are met, we need this
>>>>>>> clock to
>>>>>>> +         * be marked as critical:
>>>>>>> +         * a) Mark the clock used by a firmware resource, if
>>>>>>> possible
>>>>>>> +         * b) CCF has a clock hand-off mechanism to make the
>>>>>>> sure the
>>>>>>> +         *    clock stays on until the proper driver comes along
>>>>>>> +         */
>>>>>>> +        .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 = 24,
>>>>>>> +    },
>>>>>>> +    .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,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +/* Array of all clocks provided by this provider */
>>>>>>> +static struct clk_hw_onecell_data a1_pll_hw_onecell_data = {
>>>>>>> +    .hws = {
>>>>>>> +        [CLKID_FIXED_PLL_DCO]        = &a1_fixed_pll_dco.hw,
>>>>>>> +        [CLKID_FIXED_PLL]        = &a1_fixed_pll.hw,
>>>>>>> +        [CLKID_HIFI_PLL]        = &a1_hifi_pll.hw,
>>>>>>> +        [CLKID_FCLK_DIV2]        = &a1_fclk_div2.hw,
>>>>>>> +        [CLKID_FCLK_DIV3]        = &a1_fclk_div3.hw,
>>>>>>> +        [CLKID_FCLK_DIV5]        = &a1_fclk_div5.hw,
>>>>>>> +        [CLKID_FCLK_DIV7]        = &a1_fclk_div7.hw,
>>>>>>> +        [CLKID_FCLK_DIV2_DIV]        = &a1_fclk_div2_div.hw,
>>>>>>> +        [CLKID_FCLK_DIV3_DIV]        = &a1_fclk_div3_div.hw,
>>>>>>> +        [CLKID_FCLK_DIV5_DIV]        = &a1_fclk_div5_div.hw,
>>>>>>> +        [CLKID_FCLK_DIV7_DIV]        = &a1_fclk_div7_div.hw,
>>>>>>> +        [NR_PLL_CLKS]            = NULL,
>>>>>>> +    },
>>>>>>> +    .num = NR_PLL_CLKS,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap *const a1_pll_regmaps[] = {
>>>>>>> +    &a1_fixed_pll_dco,
>>>>>>> +    &a1_fixed_pll,
>>>>>>> +    &a1_hifi_pll,
>>>>>>> +    &a1_fclk_div2,
>>>>>>> +    &a1_fclk_div3,
>>>>>>> +    &a1_fclk_div5,
>>>>>>> +    &a1_fclk_div7,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int meson_a1_pll_probe(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ret = meson_eeclkc_probe(pdev);
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>
>>>>>> This function is useless.
>>>>>>
>>>>> OK, I will use meson_eeclkc_probe derectly.
>>>>>>> +
>>>>>>> +static const struct meson_eeclkc_data a1_pll_data = {
>>>>>>> +        .regmap_clks = a1_pll_regmaps,
>>>>>>> +        .regmap_clk_num = ARRAY_SIZE(a1_pll_regmaps),
>>>>>>> +        .hw_onecell_data = &a1_pll_hw_onecell_data,
>>>>>>> +};
>>>>>>> +static const struct of_device_id clkc_match_table[] = {
>>>>>>> +    {
>>>>>>> +        .compatible = "amlogic,a1-pll-clkc",
>>>>>>> +        .data = &a1_pll_data
>>>>>>> +    },
>>>>>>> +    { /* sentinel */ }
>>>>>>
>>>>>> Nitpick: don't need to write this, just write the line like this
>>>>>>
>>>>> OK, remove it.
>>>>>> ' }, {}'
>>>>>>
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct platform_driver a1_driver = {
>>>>>>> +    .probe        = meson_a1_pll_probe,
>>>>>>> +    .driver        = {
>>>>>>> +        .name    = "a1-pll-clkc",
>>>>>>> +        .of_match_table = clkc_match_table,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +builtin_platform_driver(a1_driver);
>>>>>>> diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000..99ee2a9
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/clk/meson/a1-pll.h
>>>>>>> @@ -0,0 +1,56 @@
>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>>>>>>> +/*
>>>>>>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef __A1_PLL_H
>>>>>>> +#define __A1_PLL_H
>>>>>>> +
>>>>>>> +/* PLL register offset */
>>>>>>> +#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
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * CLKID index values
>>>>>>> + *
>>>>>>> + * These indices are entirely contrived and do not map onto the
>>>>>>> hardware.
>>>>>>> + * It has now been decided to expose everything by default in
>>>>>>> the DT header:
>>>>>>> + * include/dt-bindings/clock/a1-pll-clkc.h. Only the clocks ids
>>>>>>> we don't want
>>>>>>> + * to expose, such as the internal muxes and dividers of
>>>>>>> composite clocks,
>>>>>>> + * will remain defined here.
>>>>>>> + */
>>>>>>> +#define CLKID_FIXED_PLL_DCO        0
>>>>>>> +#define CLKID_FCLK_DIV2_DIV        2
>>>>>>> +#define CLKID_FCLK_DIV3_DIV        3
>>>>>>> +#define CLKID_FCLK_DIV5_DIV        4
>>>>>>> +#define CLKID_FCLK_DIV7_DIV        5
>>>>>>> +#define NR_PLL_CLKS            11
>>>>>>> +
>>>>>>> +/* include the CLKIDs that have been made part of the DT binding */
>>>>>>> +#include <dt-bindings/clock/a1-pll-clkc.h>
>>>>>>> +
>>>>>>> +#endif /* __A1_PLL_H */
>>>>>>> diff --git a/drivers/clk/meson/a1.c b/drivers/clk/meson/a1.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..86a4733
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/clk/meson/a1.c
>>>>>>> @@ -0,0 +1,2264 @@
>>>>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>>>>> +/*
>>>>>>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>>>>>>> + * Author: Jian Hu <jian.hu@amlogic.com>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <linux/platform_device.h>
>>>>>>> +#include "clk-pll.h"
>>>>>>> +#include "clk-dualdiv.h"
>>>>>>> +#include "meson-eeclk.h"
>>>>>>> +#include "a1.h"
>>>>>>
>>>>>> Same as above
>>>>> OK, I will change the order.
>>>>> In fact, the clk-pll.h is not used in the current driver.
>>>>> I will remove it.
>>>>>>
>>>>>>> +
>>>>>>> +/* 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_ro_ops,
>>>>>>> +        .parent_data = &(const struct clk_parent_data) {
>>>>>>> +            .fw_name = "xtal",
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        /*
>>>>>>> +         * switch for xtal clock
>>>>>>> +         * Linux should not change it at runtime
>>>>>>> +         */
>>>>>>
>>>>>> Comment not useful: it uses the Ro ops
>>>>>>
>>>>> OK,  remove the comments
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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,
>>>>>>> +        /*
>>>>>>> +         * it feeds DDR,AXI,APB bus
>>>>>>> +         * Linux should not change it at runtime
>>>>>>> +         */
>>>>>>
>>>>>> Again, the child are critical, not directly this clock from your
>>>>>> comment.
>>>>>>
>>>>>> Remove CRITICAL, put RO is linux is not supposed to touch it.
>>>>>>
>>>>> repace as clk_regmap_gate_ro_ops
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>> +
>>>>>>> +/* 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", },
>>>>>>> +    { .fw_name = "fclk_div2", },
>>>>>>> +    { .fw_name = "fclk_div3", },
>>>>>>> +    { .fw_name = "fclk_div5", },
>>>>>>> +    { .fw_name = "hifi_pll", },
>>>>>>> +    { .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),
>>>>>>
>>>>>> .flags = CLK_SET_RATE_PARENT ?
>>>>> Yes, I miss the flag.
>>>>>>
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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),
>>>>>>
>>>>>> .flags = CLK_SET_RATE_PARENT ?
>>>>>>
>>>>> Yes, I will add it.
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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,
>>>>>>
>>>>>> Why do you need CLK_IGNORE_UNUSED ?
>>>>>>
>>>>> I should remove it
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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,
>>>>>>
>>>>>> Why do you need CLK_IGNORE_UNUSED ?
>>>>>>
>>>>> I should remove it
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>
>>>>>> Same question and remarks applies to DSP B
>>>>>>
>>>>> got it
>>>>>>> +/* 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_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 },
>>>>>>> +};
>>>>>>
>>>>>> Looks like the same as SAR ADC
>>>>>>
>>>>> OK, I will describe it like SAR ADC for pwm a/b/c/d
>>>>>>> +
>>>>>>> +/* 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,
>>>>>>> +        /*
>>>>>>> +         * The CPU working voltage is controlled by pwm_a
>>>>>>> +         * in BL2 firmware. add the CLK_IGNORE_UNUSED flag
>>>>>>> +         * to avoid changing at runtime.
>>>>>>                                        ^ it
>>>>>>
>>>>>>> +         * and is required by the platform to operate correctly.
>>>>>>                       "blabla. And" is strange
>>>>>>
>>>>>>> +         * Until the following condition are met, we need this
>>>>>>> clock to
>>>>>>> +         * be marked as critical:
>>>>>>> +         * a) Mark the clock used by a firmware resource, if
>>>>>>> possible
>>>>>>> +         * b) CCF has a clock hand-off mechanism to make the
>>>>>>> sure the
>>>>>>> +         *    clock stays on until the proper driver comes along
>>>>>>> +         */
>>>>>>> +        .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>>>>
>>>>>> This only skips the late_init() disable of unused clocks
>>>>>>
>>>>>> Be aware that this is not fool-proof. If at any time a driver enable
>>>>>> then disable the clock, the clock will be disable and I guess your
>>>>>> platform will die if this provides the CPU voltage.
>>>>> OK, CLK_IS_CRITICAL is better.
>>>>>>
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +/* pwm b clk */
>>>>>>> +static struct clk_regmap a1_pwm_b_sel = {
>>>>>>> +    .data = &(struct clk_regmap_mux_data){
>>>>>>> +        .offset = PWM_CLK_AB_CTRL,
>>>>>>> +        .mask = 0x1,
>>>>>>> +        .shift = 25,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_b_sel",
>>>>>>> +        .ops = &clk_regmap_mux_ops,
>>>>>>> +        .parent_data = pwm_parent_data,
>>>>>>> +        .num_parents = ARRAY_SIZE(pwm_parent_data),
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_b_div = {
>>>>>>> +    .data = &(struct clk_regmap_div_data){
>>>>>>> +        .offset = PWM_CLK_AB_CTRL,
>>>>>>> +        .shift = 16,
>>>>>>> +        .width = 8,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_b_div",
>>>>>>> +        .ops = &clk_regmap_divider_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_b_sel.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_b = {
>>>>>>> +    .data = &(struct clk_regmap_gate_data){
>>>>>>> +        .offset = PWM_CLK_AB_CTRL,
>>>>>>> +        .bit_idx = 24,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data) {
>>>>>>> +        .name = "pwm_b",
>>>>>>> +        .ops = &clk_regmap_gate_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_b_div.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +/* pwm c clk */
>>>>>>> +static struct clk_regmap a1_pwm_c_sel = {
>>>>>>> +    .data = &(struct clk_regmap_mux_data){
>>>>>>> +        .offset = PWM_CLK_CD_CTRL,
>>>>>>> +        .mask = 0x1,
>>>>>>> +        .shift = 9,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_c_sel",
>>>>>>> +        .ops = &clk_regmap_mux_ops,
>>>>>>> +        .parent_data = pwm_parent_data,
>>>>>>> +        .num_parents = ARRAY_SIZE(pwm_parent_data),
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_c_div = {
>>>>>>> +    .data = &(struct clk_regmap_div_data){
>>>>>>> +        .offset = PWM_CLK_CD_CTRL,
>>>>>>> +        .shift = 0,
>>>>>>> +        .width = 8,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_c_div",
>>>>>>> +        .ops = &clk_regmap_divider_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_c_sel.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_c = {
>>>>>>> +    .data = &(struct clk_regmap_gate_data){
>>>>>>> +        .offset = PWM_CLK_CD_CTRL,
>>>>>>> +        .bit_idx = 8,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data) {
>>>>>>> +        .name = "pwm_c",
>>>>>>> +        .ops = &clk_regmap_gate_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_c_div.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +/* pwm d clk */
>>>>>>> +static struct clk_regmap a1_pwm_d_sel = {
>>>>>>> +    .data = &(struct clk_regmap_mux_data){
>>>>>>> +        .offset = PWM_CLK_CD_CTRL,
>>>>>>> +        .mask = 0x1,
>>>>>>> +        .shift = 25,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_d_sel",
>>>>>>> +        .ops = &clk_regmap_mux_ops,
>>>>>>> +        .parent_data = pwm_parent_data,
>>>>>>> +        .num_parents = ARRAY_SIZE(pwm_parent_data),
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_d_div = {
>>>>>>> +    .data = &(struct clk_regmap_div_data){
>>>>>>> +        .offset = PWM_CLK_CD_CTRL,
>>>>>>> +        .shift = 16,
>>>>>>> +        .width = 8,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_d_div",
>>>>>>> +        .ops = &clk_regmap_divider_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_d_sel.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_d = {
>>>>>>> +    .data = &(struct clk_regmap_gate_data){
>>>>>>> +        .offset = PWM_CLK_CD_CTRL,
>>>>>>> +        .bit_idx = 24,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data) {
>>>>>>> +        .name = "pwm_d",
>>>>>>> +        .ops = &clk_regmap_gate_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_d_div.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct clk_parent_data pwm_ef_parent_data[] = {
>>>>>>> +    { .fw_name = "xtal", },
>>>>>>> +    { .hw = &a1_sys_clk.hw },
>>>>>>> +    { .fw_name = "fclk_div5", },
>>>>>>> +    { .hw = &a1_rtc_clk.hw },
>>>>>>> +};
>>>>>>> +
>>>>>>> +/* pwm e clk */
>>>>>>> +static struct clk_regmap a1_pwm_e_sel = {
>>>>>>> +    .data = &(struct clk_regmap_mux_data){
>>>>>>> +        .offset = PWM_CLK_EF_CTRL,
>>>>>>> +        .mask = 0x3,
>>>>>>> +        .shift = 9,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_e_sel",
>>>>>>> +        .ops = &clk_regmap_mux_ops,
>>>>>>> +        .parent_data = pwm_ef_parent_data,
>>>>>>> +        .num_parents = ARRAY_SIZE(pwm_ef_parent_data),
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_e_div = {
>>>>>>> +    .data = &(struct clk_regmap_div_data){
>>>>>>> +        .offset = PWM_CLK_EF_CTRL,
>>>>>>> +        .shift = 0,
>>>>>>> +        .width = 8,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_e_div",
>>>>>>> +        .ops = &clk_regmap_divider_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_e_sel.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_e = {
>>>>>>> +    .data = &(struct clk_regmap_gate_data){
>>>>>>> +        .offset = PWM_CLK_EF_CTRL,
>>>>>>> +        .bit_idx = 8,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data) {
>>>>>>> +        .name = "pwm_e",
>>>>>>> +        .ops = &clk_regmap_gate_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_e_div.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +/* pwm f clk */
>>>>>>> +static struct clk_regmap a1_pwm_f_sel = {
>>>>>>> +    .data = &(struct clk_regmap_mux_data){
>>>>>>> +        .offset = PWM_CLK_EF_CTRL,
>>>>>>> +        .mask = 0x3,
>>>>>>> +        .shift = 25,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_f_sel",
>>>>>>> +        .ops = &clk_regmap_mux_ops,
>>>>>>> +        .parent_data = pwm_ef_parent_data,
>>>>>>> +        .num_parents = ARRAY_SIZE(pwm_ef_parent_data),
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_f_div = {
>>>>>>> +    .data = &(struct clk_regmap_div_data){
>>>>>>> +        .offset = PWM_CLK_EF_CTRL,
>>>>>>> +        .shift = 16,
>>>>>>> +        .width = 8,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>> +        .name = "pwm_f_div",
>>>>>>> +        .ops = &clk_regmap_divider_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_f_sel.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct clk_regmap a1_pwm_f = {
>>>>>>> +    .data = &(struct clk_regmap_gate_data){
>>>>>>> +        .offset = PWM_CLK_EF_CTRL,
>>>>>>> +        .bit_idx = 24,
>>>>>>> +    },
>>>>>>> +    .hw.init = &(struct clk_init_data) {
>>>>>>> +        .name = "pwm_f",
>>>>>>> +        .ops = &clk_regmap_gate_ops,
>>>>>>> +        .parent_hws = (const struct clk_hw *[]) {
>>>>>>> +            &a1_pwm_f_div.hw
>>>>>>> +        },
>>>>>>> +        .num_parents = 1,
>>>>>>> +        .flags = CLK_SET_RATE_PARENT,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>> +
>>>>>>> +/* dmc clk */
>>>>>>> +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_data = sd_emmc_parents,
>>>>>>> +        .num_parents = 4,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +
>>>>>>> +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,
>>>>>>> +        /*
>>>>>>> +         * This clock is used by DDR clock which setted in BL2
>>>>>>> +         * and is required by the platform to operate correctly.
>>>>>>> +         * Until the following condition are met, we need this
>>>>>>> clock to
>>>>>>> +         * be marked as critical:
>>>>>>> +         * a) Mark the clock used by a firmware resource, if
>>>>>>> possible
>>>>>>> +         * b) CCF has a clock hand-off mechanism to make the
>>>>>>> sure the
>>>>>>> +         *    clock stays on until the proper driver comes along
>>>>>>> +         */
>>>>>>> +        .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>>>>>> +    },
>>>>>>> +};
>>>>>>
>>>>>> Should you put all this DMC stuff in RO until you got a driver for
>>>>>> it ?
>>>>> OK, replace as clk_regmap_gate_ro_ops
>>>>>>
>>>>> [ ... ]
>>>>>>> +
>>>>>>> +static int meson_a1_periphs_probe(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ret = meson_eeclkc_probe(pdev);
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>
>>>>>> Again this function is function is useless and it makes me wonder
>>>>>> if you
>>>>>> should really be using meson_eeclkc_probe()
>>>>>>
>>>>>> This makes you use syscon which is not correct unless you have a good
>>>>>> reason ?
>>>>>>
>>>>> If it can not use the meson_eeclkc_probe(), I will realize a probe
>>>>> function
>>>>> which is mostly duplicate with meson_eeclkc_probe() except
>>>>> "syscon_node_to_regmap"
>>>>>
>>>>> Maybe another common probe function and a new file are required for A1
>>>>> three drivers? （include the CPU clock driver）
>>>>
>>>> Maybe
>>>>
>>> I will add a new function base on meson-eeclk.c
>>>>>
>>>>> Or using meson_eeclkc_probe is more easier?
>>>>
>>>> It is not question of easiness, but correctness.
>>>>
>>>>>
>>>>>> Is there anything but clocks and resets in these register region ?
>>>>> No, there is only clocks in the register region.
>>>>> the same does the PLL register region.
>>>>
>>>> Then there is no reason to use syscon for those drivers
>>>> Add a new probe function for A1
>>>>>>
>>>>>>> +
>>>>>>> +static const struct meson_eeclkc_data a1_periphs_data = {
>>>>>>> +        .regmap_clks = a1_periphs_regmaps,
>>>>>>> +        .regmap_clk_num = ARRAY_SIZE(a1_periphs_regmaps),
>>>>>>> +        .hw_onecell_data = &a1_periphs_hw_onecell_data,
>>>>>>> +};
>>>>>>> +static const struct of_device_id clkc_match_table[] = {
>>>>>>> +    {
>>>>>>> +        .compatible = "amlogic,a1-periphs-clkc",
>>>>>>> +        .data = &a1_periphs_data
>>>>>>> +    },
>>>>>>> +    { /* sentinel */ }
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct platform_driver a1_driver = {
>>>>>>> +    .probe        = meson_a1_periphs_probe,
>>>>>>> +    .driver        = {
>>>>>>> +        .name    = "a1-periphs-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..1ae5e04
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/clk/meson/a1.h
>>>>>>> @@ -0,0 +1,120 @@
>>>>>>> +/* 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 offset */
>>>>>>> +#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
>>>>>>> +
>>>>>>> +#define CLKID_XTAL_CLKTREE        0
>>>>>>> +#define CLKID_SYS_A_SEL            89
>>>>>>> +#define CLKID_SYS_A_DIV            90
>>>>>>> +#define CLKID_SYS_A            91
>>>>>>> +#define CLKID_SYS_B_SEL            92
>>>>>>> +#define CLKID_SYS_B_DIV            93
>>>>>>> +#define CLKID_SYS_B            94
>>>>>>> +#define CLKID_DSPA_A_SEL        95
>>>>>>> +#define CLKID_DSPA_A_DIV        96
>>>>>>> +#define CLKID_DSPA_A            97
>>>>>>> +#define CLKID_DSPA_B_SEL        98
>>>>>>> +#define CLKID_DSPA_B_DIV        99
>>>>>>> +#define CLKID_DSPA_B            100
>>>>>>> +#define CLKID_DSPB_A_SEL        101
>>>>>>> +#define CLKID_DSPB_A_DIV        102
>>>>>>> +#define CLKID_DSPB_A            103
>>>>>>> +#define CLKID_DSPB_B_SEL        104
>>>>>>> +#define CLKID_DSPB_B_DIV        105
>>>>>>> +#define CLKID_DSPB_B            106
>>>>>>> +#define CLKID_RTC_32K_CLKIN        107
>>>>>>> +#define CLKID_RTC_32K_DIV        108
>>>>>>> +#define CLKID_RTC_32K_XTAL        109
>>>>>>> +#define CLKID_RTC_32K_SEL        110
>>>>>>> +#define CLKID_CECB_32K_CLKIN        111
>>>>>>> +#define CLKID_CECB_32K_DIV        112
>>>>>>> +#define CLKID_CECB_32K_SEL_PRE        113
>>>>>>> +#define CLKID_CECB_32K_SEL        114
>>>>>>> +#define CLKID_CECA_32K_CLKIN        115
>>>>>>> +#define CLKID_CECA_32K_DIV        116
>>>>>>> +#define CLKID_CECA_32K_SEL_PRE        117
>>>>>>> +#define CLKID_CECA_32K_SEL        118
>>>>>>> +#define CLKID_DIV2_PRE            119
>>>>>>> +#define CLKID_24M_DIV2            120
>>>>>>> +#define CLKID_GEN_SEL            121
>>>>>>> +#define CLKID_GEN_DIV            122
>>>>>>> +#define CLKID_SARADC_DIV        123
>>>>>>> +#define CLKID_PWM_A_SEL            124
>>>>>>> +#define CLKID_PWM_A_DIV            125
>>>>>>> +#define CLKID_PWM_B_SEL            126
>>>>>>> +#define CLKID_PWM_B_DIV            127
>>>>>>> +#define CLKID_PWM_C_SEL            128
>>>>>>> +#define CLKID_PWM_C_DIV            129
>>>>>>> +#define CLKID_PWM_D_SEL            130
>>>>>>> +#define CLKID_PWM_D_DIV            131
>>>>>>> +#define CLKID_PWM_E_SEL            132
>>>>>>> +#define CLKID_PWM_E_DIV            133
>>>>>>> +#define CLKID_PWM_F_SEL            134
>>>>>>> +#define CLKID_PWM_F_DIV            135
>>>>>>> +#define CLKID_SPICC_SEL            136
>>>>>>> +#define CLKID_SPICC_DIV            137
>>>>>>> +#define CLKID_SPICC_SEL2        138
>>>>>>> +#define CLKID_TS_DIV            139
>>>>>>> +#define CLKID_SPIFC_SEL            140
>>>>>>> +#define CLKID_SPIFC_DIV            141
>>>>>>> +#define CLKID_SPIFC_SEL2        142
>>>>>>> +#define CLKID_USB_BUS_SEL        143
>>>>>>> +#define CLKID_USB_BUS_DIV        144
>>>>>>> +#define CLKID_SD_EMMC_SEL        145
>>>>>>> +#define CLKID_SD_EMMC_DIV        146
>>>>>>> +#define CLKID_SD_EMMC_SEL2        147
>>>>>>> +#define CLKID_PSRAM_SEL            148
>>>>>>> +#define CLKID_PSRAM_DIV            149
>>>>>>> +#define CLKID_PSRAM_SEL2        150
>>>>>>> +#define CLKID_DMC_SEL            151
>>>>>>> +#define CLKID_DMC_DIV            152
>>>>>>> +#define CLKID_DMC_SEL2            153
>>>>>>> +#define NR_CLKS                154
>>>>>>> +
>>>>>>> +#include <dt-bindings/clock/a1-clkc.h>
>>>>>>> +
>>>>>>> +#endif /* __A1_H */
>>>>>>
>>>>>> .
>>>>>>
>>>>
>>>> .
>>>>
>>
>> .
>>