Hi, Jerome sorry, it's my problem. I did not discribe clearly the issue. On 2019/10/11 15:39, Jerome Brunet wrote:
scene，it will get failed because of the clock not being reigstered. In fact, we could not use devm_clk_get.On Tue 08 Oct 2019 at 10:03, Jian Hu <jian.hu@xxxxxxxxxxx> wrote: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.What does "fail" mean ?I intended to get clock using devm_clk_get API in each driver, In this
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.Those are work arounds. Actually fixing the problem is usually preferable. So if rephrase your problem: * We have 2 clock controllers (A and B) * Clock are passed between the controllers using DT * We have a PLL in controller B which is used by clocks in controller A. * the PLL parent clock is in controller A.
Yeah, it is the scene.
=> So if I understand correctly you are saying that it will "fail" because there is a circular dependency between controller A and B, right ? Do you have evidence that your problem comes from this circular dependency ?
I have realized the peripheral driver and PLL drivers, PLL driver probes first, Peripheral clock driver is the second. In addition，for A1 SoC， it will not work using meson_clk_pll_ops,it needs strictly sequence，so maybe another ops is required.hifi pll will be sent with sys pll and CPU clock driver.
AFAIK, CCF will orphan the clock and continue if the parent is not available. Later, when the parent comes up, the orphan will be reparented. IOW, the problem you are reporting should already be covered by CCF.And which sulution is better above two?Neither, I'm afraidOr maybe other good ideas for it?My bet would be that an important clocks (maybe more than 1) is being gated during the init process. Maybe you should try the command line parameter "clk_ignore_unused" until you get things running with your 2 controllers.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 which might be desirable for audio use case. Why is this one restricted to one particular rate ?The audio working frequency are 44.1khz, 48khz and 192khz. 614.4M can meet the three frequency. after the hifi pll, there are two dividers in Audio clock. 614.4M/3200 = 192khz 614.4M/12800 = 48khz 614,4M/13932 = 44.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...
