Re: [RFC/RFT v3 07/14] clk: meson: g12a: add notifiers to handle cpu clock change
Quoting Martin Blumenstingl (2019-07-02 16:28:55) > Hi Stephen, Hi Neil, > > On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong > 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? > Derek told me yesterday he wants to work on it again, but I don't know his timeline. If Derek doesn't reply here then maybe it can be picked up by someone else.
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
> 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 = 0x },
>> - { .reg = HHI_SYS_PLL_CNTL2, .def = 0x },
>> - { .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 = 0x5654 },
>> +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 = _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 = _clk_pll_ro_ops,
>> + .ops = _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/
>
Re: [RFC/RFT v3 07/14] clk: meson: g12a: add notifiers to handle cpu clock change
Hi Stephen, Hi Neil,
On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong 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 = 0x },
> - { .reg = HHI_SYS_PLL_CNTL2, .def = 0x },
> - { .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 = 0x5654 },
> +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 = _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 = _clk_pll_ro_ops,
> + .ops = _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/
