Hi, Jerome PLL clocks and peripheral clocks rely on each other. for fixed_pll, we can describe its parent like this: xtal-->xtal_fixpll-->fixed_dco-->fixed_pll xtal fixpll is belong to peripheral region. fixed_pll/fclk_div2/fclk_div3 is belong to PLL region. if PLL clocks probe first, it will fail to get xtal_fixpll. we can not get fixed_dco's parent clock. if peripheral clocks probe first, it will fail to get fixed_pll clocks. A lot of peripheral clocks parent are fclk_div2/3/5/7. and we can not get fclk_div2/3/5/7 clocks. I can think of two solutions: 1) Do not describe xtal_fixpll, xtal_hifipll. that is to say, do not decribe the SYS_OSCIN_CTRL register. 2) Put peripheral and pll clock driver in one driver. And which sulution is better above two? Or maybe other good ideas for it? On 2019/9/29 17:38, Jian Hu wrote:
On 2019/9/27 21:32, Jerome Brunet wrote:On Fri 27 Sep 2019 at 11:52, Jian Hu <jian.hu@xxxxxxxxxxx> wrote:Hi, Jerome Thank you for review. On 2019/9/25 23:09, Jerome Brunet wrote:On Wed 25 Sep 2019 at 19:44, Jian Hu <jian.hu@xxxxxxxxxxx> wrote:The Amlogic A1 clock includes three parts: peripheral clocks, pll clocks, CPU clocks. sys pll and CPU clocks will be sent in next patch. Unlike the previous series, there is no EE/AO domain in A1 CLK controllers. Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx> Signed-off-by: Jianxin Pan <jianxin.pan@xxxxxxxxxxx> --- arch/arm64/Kconfig.platforms | 1 + drivers/clk/meson/Kconfig | 10 + drivers/clk/meson/Makefile | 1 +drivers/clk/meson/a1.c | 2617 ++++++++++++++++++++++++++++++++++++++++++drivers/clk/meson/a1.h | 172 +++ 5 files changed, 2801 insertions(+) create mode 100644 drivers/clk/meson/a1.c create mode 100644 drivers/clk/meson/a1.h
[...]
diff --git a/drivers/clk/meson/a1.c b/drivers/clk/meson/a1.c new file mode 100644 index 0000000..26edae0f --- /dev/null +++ b/drivers/clk/meson/a1.c @@ -0,0 +1,2617 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (c) 2019 Amlogic, Inc. All rights reserved. + */ + +#include <linux/clk-provider.h> +#include <linux/init.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/of_address.h> +#include "clk-mpll.h" +#include "clk-pll.h" +#include "clk-regmap.h" +#include "vid-pll-div.h" +#include "clk-dualdiv.h" +#include "meson-eeclk.h" +#include "a1.h" + +/* PLLs clock in gates, its parent is xtal */ +static struct clk_regmap a1_xtal_clktree = { + .data = &(struct clk_regmap_gate_data){ + .offset = SYS_OSCIN_CTRL, + .bit_idx = 0, + }, + .hw.init = &(struct clk_init_data) { + .name = "xtal_clktree", + .ops = &clk_regmap_gate_ops, + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xtal", + }, + .num_parents = 1, + .flags = CLK_IS_CRITICAL,Is CCF even expected to touch this ever ? what about RO ops ? Please review your other clocks with this in mindthe clock should not be changed at runtime.clk_regmap_gate_ro_ops is a good idea. Set RO ops and remove the CLK_IS_CRITICAL flag.+ }, +}; + +static struct clk_regmap a1_xtal_fixpll = { + .data = &(struct clk_regmap_gate_data){ + .offset = SYS_OSCIN_CTRL, + .bit_idx = 1, + }, + .hw.init = &(struct clk_init_data) { + .name = "xtal_fixpll", + .ops = &clk_regmap_gate_ops, + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xtal", + }, + .num_parents = 1, + .flags = CLK_IS_CRITICAL, + }, +}; + +static struct clk_regmap a1_xtal_usb_phy = { + .data = &(struct clk_regmap_gate_data){ + .offset = SYS_OSCIN_CTRL, + .bit_idx = 2, + }, + .hw.init = &(struct clk_init_data) { + .name = "xtal_usb_phy", + .ops = &clk_regmap_gate_ops, + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xtal", + }, + .num_parents = 1, + .flags = CLK_IS_CRITICAL,How is an USB clock critical to the system ? Please review your other clocks with comment in mind ...the usb clock does not affect the system, remove the CLK_IS_CRITICAL flag+ }, +}; + +static struct clk_regmap a1_xtal_usb_ctrl = { + .data = &(struct clk_regmap_gate_data){ + .offset = SYS_OSCIN_CTRL, + .bit_idx = 3, + }, + .hw.init = &(struct clk_init_data) { + .name = "xtal_usb_ctrl", + .ops = &clk_regmap_gate_ops, + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xtal", + }, + .num_parents = 1, + .flags = CLK_IS_CRITICAL, + }, +};the usb clock does not affect the system, remove the CLK_IS_CRITICAL flag+ +static struct clk_regmap a1_xtal_hifipll = { + .data = &(struct clk_regmap_gate_data){ + .offset = SYS_OSCIN_CTRL, + .bit_idx = 4, + }, + .hw.init = &(struct clk_init_data) { + .name = "xtal_hifipll", + .ops = &clk_regmap_gate_ops, + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xtal", + }, + .num_parents = 1, + .flags = CLK_IS_CRITICAL, + }, +};CLK_IS_CRITICAL is need to lock hifi pll.That's not how CCF works, this falg is not ok here. CCF will enable this clock before calling enable on your hifi pllok， I will remove it.+ +static struct clk_regmap a1_xtal_syspll = { + .data = &(struct clk_regmap_gate_data){ + .offset = SYS_OSCIN_CTRL, + .bit_idx = 5, + }, + .hw.init = &(struct clk_init_data) { + .name = "xtal_syspll", + .ops = &clk_regmap_gate_ops, + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xtal", + }, + .num_parents = 1, + .flags = CLK_IS_CRITICAL, + }, +}; +when CPU clock is at fixed clock, sys pll will be disabled, xtal_syspll will be disabled too. when setting sys pll, call clk_set_rate to lock sys pll, add RO ops to avoid disabling the clockAgain not Ok. If you mechanism to lock the PLL is properly implemented in the enable callback of the sys pll, still kind of work around are not needed This has worked on the pll we had so far.ok, I will remove it.+static struct clk_regmap a1_xtal_dds = { + .data = &(struct clk_regmap_gate_data){ + .offset = SYS_OSCIN_CTRL, + .bit_idx = 6, + }, + .hw.init = &(struct clk_init_data) { + .name = "xtal_dds", + .ops = &clk_regmap_gate_ops, + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xtal", + }, + .num_parents = 1, + .flags = CLK_IS_CRITICAL, + }, +};CLK_IS_CRITICAL is need to lock dds+ +/* fixed pll = 1536M + * + * fixed pll ----- fclk_div2 = 768M + * | + * ----- fclk_div3 = 512M + * | + * ----- fclk_div5 = 307.2M + * | + * ----- fclk_div7 = 219.4M + */The framework will make those calculation ... you can remove thisok, I will remote the comment.+static struct clk_regmap a1_fixed_pll_dco = { + .data = &(struct meson_clk_pll_data){ + .en = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 28, + .width = 1, + }, + .m = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 0, + .width = 8, + }, + .n = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 10, + .width = 5, + }, + .frac = { + .reg_off = ANACTRL_FIXPLL_CTRL1, + .shift = 0, + .width = 19, + }, + .l = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 31, + .width = 1, + }, + .rst = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 29, + .width = 1, + }, + }, + .hw.init = &(struct clk_init_data){ + .name = "fixed_pll_dco", + .ops = &meson_clk_pll_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_xtal_fixpll.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap a1_fixed_pll = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL0, + .bit_idx = 20, + }, + .hw.init = &(struct clk_init_data) { + .name = "fixed_pll", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fixed_pll_dco.hw + }, + .num_parents = 1, + .flags = CLK_IGNORE_UNUSED, + }, +}; + +static const struct pll_params_table a1_hifi_pll_params_table[] = { + PLL_PARAMS(128, 5), /* DCO = 614.4M */ +}; + +static const struct reg_sequence a1_hifi_init_regs[] = { + { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 }, + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 }, + { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x10022200 }, + { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00301000 }, + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f19480 },+ { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f19480, .delay_us = 10 }, + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f11480, .delay_us = 40 },+ { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 }, + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 }, +}; + +/* + * The Meson A1 HIFI PLL is 614.4M, it requires + * a strict register sequence to enable the PLL. + * set meson_clk_pcie_pll_ops as its ops + */Could you elaborate on this ? What need to be done to enable the clock ?Also the HIFI PLL used to be able to do a *LOT* of different rate whichmight be desirable for audio use case. Why is this one restricted to oneparticular rate ?The audio working frequency are 44.1khz, 48khz and 192khz. 614.4M can meet the three frequency. after the hifi pll, there are two dividers in Audio clock. 614.4M/3200 = 192khz 614.4M/12800 = 48khz 614,4M/13932 = 44.0999khzIt does not really answer my question though. You are locking a use case here, which is 32 bit sample width We have other constraint with the upstream audio driver, and we usually looking for base frequency that a multiple of 768 (24*32). If you need your PLL to be set to a particular rate for a use case, the correct way is "assigned-rate" in DT so the question still stands, the HIFI pll before was pretty easy to set at a wide variety of rate (same as GP0) ... is it not the case anymore ? If yes, could you decribe the constraints. All this took us a long time to figure out on our own, which is why I'd prefer to get the proper constraints in from the beginning this timeok, I will verify it and describe the constraints about it+static struct clk_regmap a1_hifi_pll = { + .data = &(struct meson_clk_pll_data){ + .en = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 28, + .width = 1, + }, + .m = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 0, + .width = 8, + }, + .n = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 10, + .width = 5, + }, + .frac = { + .reg_off = ANACTRL_HIFIPLL_CTRL1, + .shift = 0, + .width = 19, + }, + .l = { + .reg_off = ANACTRL_HIFIPLL_STS, + .shift = 31, + .width = 1, + }, + .table = a1_hifi_pll_params_table, + .init_regs = a1_hifi_init_regs, + .init_count = ARRAY_SIZE(a1_hifi_init_regs), + }, + .hw.init = &(struct clk_init_data){ + .name = "hifi_pll", + .ops = &meson_clk_pcie_pll_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_xtal_hifipll.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_fixed_factor a1_fclk_div2_div = { + .mult = 1, + .div = 2, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div2_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fixed_pll.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap a1_fclk_div2 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL0, + .bit_idx = 21, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div2", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fclk_div2_div.hw + }, + .num_parents = 1, + /*+ * add CLK_IS_CRITICAL flag to avoid being disabled by clk core+ * or its children clocks. + */The meaning of this flag is already documented in clk-provider.h The reason why you need this flag is lot more interesting here ... Same belowok, I will replace new comments here.+ .flags = CLK_IS_CRITICAL, + }, +}; + +static struct clk_fixed_factor a1_fclk_div3_div = { + .mult = 1, + .div = 3, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div3_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fixed_pll.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap a1_fclk_div3 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL0, + .bit_idx = 22, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div3", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fclk_div3_div.hw + }, + .num_parents = 1, + /*+ * add CLK_IS_CRITICAL flag to avoid being disabled by clk core+ * its children clocks. + */ + .flags = CLK_IS_CRITICAL, + }, +}; + +static struct clk_fixed_factor a1_fclk_div5_div = { + .mult = 1, + .div = 5, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div5_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fixed_pll.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap a1_fclk_div5 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL0, + .bit_idx = 23, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div5", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fclk_div5_div.hw + }, + .num_parents = 1, + /*+ * add CLK_IS_CRITICAL flag to avoid being disabled by clk core+ * its children clocks. + */ + .flags = CLK_IS_CRITICAL, + }, +}; + +static struct clk_fixed_factor a1_fclk_div7_div = { + .mult = 1, + .div = 7, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div7_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fixed_pll.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap a1_fclk_div7 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL0, + .bit_idx = 23, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div7", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { + &a1_fclk_div7_div.hw + }, + .num_parents = 1, + /*+ * add CLK_IS_CRITICAL flag to avoid being disabled by clk core+ * or its children clock. + */ + .flags = CLK_IS_CRITICAL, + }, +}; +
[...]
-- 1.9.1..
