Re: [RFC/RFT v3 07/14] clk: meson: g12a: add notifiers to handle cpu clock change

On 03/07/2019 01:28, Martin Blumenstingl wrote:

Hi Stephen, Hi Neil,

On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong [off-list ref] wrote:

quoted

In order to implement clock switching for the CLKID_CPU_CLK and
CLKID_CPUB_CLK, notifiers are added on specific points of the
clock tree :

cpu_clk / cpub_clk
|   \- cpu_clk_dyn
|      |  \- cpu_clk_premux0
|      |        |- cpu_clk_postmux0
|      |        |    |- cpu_clk_dyn0_div
|      |        |    \- xtal/fclk_div2/fclk_div3
|      |        \- xtal/fclk_div2/fclk_div3
|      \- cpu_clk_premux1
|            |- cpu_clk_postmux1
|            |    |- cpu_clk_dyn1_div
|            |    \- xtal/fclk_div2/fclk_div3
|            \- xtal/fclk_div2/fclk_div3
\ sys_pll / sys1_pll

This for each cluster, a single one for G12A, two for G12B.

Each cpu_clk_premux1 tree is marked as read-only and CLK_SET_RATE_NO_REPARENT,
to be used as "parking" clock in a safe clock frequency.

it seems that this is one case where the "coordinated clocks" feature
would come handy: [0]

We could still migrate over it later on.

Stephen, do you know if those patches stopped in March or if there's
still some ongoing effort to get them ready?

[...]

quoted

-/*
- * Internal sys pll emulation configuration parameters
- */
-static const struct reg_sequence g12a_sys_init_regs[] = {
-       { .reg = HHI_SYS_PLL_CNTL1,     .def = 0x00000000 },
-       { .reg = HHI_SYS_PLL_CNTL2,     .def = 0x00000000 },
-       { .reg = HHI_SYS_PLL_CNTL3,     .def = 0x48681c00 },
-       { .reg = HHI_SYS_PLL_CNTL4,     .def = 0x88770290 },
-       { .reg = HHI_SYS_PLL_CNTL5,     .def = 0x39272000 },
-       { .reg = HHI_SYS_PLL_CNTL6,     .def = 0x56540000 },
+static const struct pll_mult_range g12a_sys_pll_mult_range = {
+       .min = 128,
+       .max = 250,
 };

 static struct clk_regmap g12a_sys_pll_dco = {

@@ -124,14 +118,15 @@ static struct clk_regmap g12a_sys_pll_dco = {
                        .shift   = 29,
                        .width   = 1,
                },
-               .init_regs = g12a_sys_init_regs,
-               .init_count = ARRAY_SIZE(g12a_sys_init_regs),
+               .range = &g12a_sys_pll_mult_range,

Neil, I believe that this should be a separate patch with a
description which explains why we don't need the "init regs" anymore

Sure

quoted

        },
        .hw.init = &(struct clk_init_data){
                .name = "sys_pll_dco",
-               .ops = &meson_clk_pll_ro_ops,
+               .ops = &meson_clk_pll_ops,
                .parent_names = (const char *[]){ IN_PREFIX "xtal" },
                .num_parents = 1,
+               /* This clock feeds the CPU, avoid disabling it */
+               .flags = CLK_IS_CRITICAL,

maybe we should have a separate patch for making the CPU clock tree
mutable as well

Indeed

[...]

quoted

+/* This divider uses bit 26 to take change in account */
+static int g12b_cpub_clk_mux0_div_set_rate(struct clk_hw *hw,
+                                          unsigned long rate,
+                                          unsigned long parent_rate)
+{
+       struct clk_regmap *clk = to_clk_regmap(hw);
+       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
+       unsigned int val;
+       int ret;
+
+       ret = divider_get_val(rate, parent_rate, div->table, div->width,
+                             div->flags);
+       if (ret < 0)
+               return ret;
+
+       val = (unsigned int)ret << div->shift;
+
+       regmap_update_bits(clk->map, HHI_SYS_CPUB_CLK_CNTL,
+                          SYS_CPU_DYN_ENABLE, SYS_CPU_DYN_ENABLE);
+
+       return regmap_update_bits(clk->map, div->offset,
+                                 clk_div_mask(div->width) << div->shift |
+                                 SYS_CPU_DYN_ENABLE, val);
+};

the public S922X datasheet doesn't mention bit 26
do I understand the semantics correctly?:
- set SYS_CPU_DYN_ENABLE
- update the divider
- unset SYS_CPU_DYN_ENABLE

Exact, it's how Amlogic uses it, seems the HW takes the divider value
only on the "falling edge" of this bit !

too bad it's not a gate which we could model with
CLK_SET_RATE_GATE/CLK_SET_RATE_UNGATE

Yep, but it only works when I write the new divider value *and* I remove the bit.
It must be a glitch-free divider mechanism.

Neil

Martin

[0] https://patchwork.kernel.org/patch/10838949/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel