Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190701091258.3870-1-narmstrong@baylibre.com> <20190701091258.3870-8-narmstrong@baylibre.com>
In-Reply-To: <20190701091258.3870-8-narmstrong@baylibre.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Wed, 3 Jul 2019 01:28:55 +0200
Message-ID: <CAFBinCAnKeGYkyCmEMugWuQaSxgBp4DqtHN3b0rLJY6jwOF0QA@mail.gmail.com>
Subject: Re: [RFC/RFT v3 07/14] clk: meson: g12a: add notifiers to handle cpu
 clock change
To:     sboyd@codeaurora.org, Neil Armstrong <narmstrong@baylibre.com>
Cc:     jbrunet@baylibre.com, khilman@baylibre.com,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Stephen, Hi Neil,

On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> 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]
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

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

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

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


Martin

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