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

Jian Hu
2019-11-09T11:17:35Z

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

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