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

[Neil Armstrong <narmstrong@xxxxxxxxxxxx>]

On 03/07/2019 01:28, Martin Blumenstingl wrote:
> Hi Stephen, Hi Neil,
> 
> On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>>
>> 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?
> 
> [...]
>> -/*
>> - * 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

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

> 
> [...]
>> +/* 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/
>