Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks

[Neil Armstrong]

Hi Martin,

On 01/03/2019 16:21, Martin Blumenstingl wrote:
> Hi Neil,
> 
> it's great to see the progress on G12A!
> 
> On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>>
>> Add the Amlogic G12A Family CPU Clock tree in read/only for now.
>>
>> The CPU clock can either use the SYS_PLL for > 1GHz frequencies or
>> use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch
>> muxes.
>>
>> Proper DVFS support will come in a second time.
> can you please also mention that this adds various CPU clock
> post-dividers (APB, ATB, AXI and CPU trace)?
> I don't mind them being int his patchset

indeed, I forgot this !

> 
> disclaimer for my code-review:
> - I don't have access to the datasheet so I can't verify if the clock
> tree from this patch is correct
> - the latest buildroot code with G12A support
> (buildroot_openlinux_kernel_4.9_fbdev_20180706) doesn't have proper
> names for all clocks
> - based on my experience with Meson8* this looks good overall, some
> questions and comments below
> 
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>
>> ---
>>  drivers/clk/meson/g12a.c | 348 +++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/meson/g12a.h |   1 +
>>  2 files changed, 349 insertions(+)
>>
>> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
>> index 0e1ce8c03259..4c938f1b8421 100644
>> --- a/drivers/clk/meson/g12a.c
>> +++ b/drivers/clk/meson/g12a.c
>> @@ -150,6 +150,316 @@ static struct clk_regmap g12a_sys_pll = {
>>         },
>>  };
>>
>> +static struct clk_regmap g12a_sys_pll_div16_en = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 24,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "sys_pll_div16_en",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "sys_pll" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is used to debug the sys_pll range
>> +                * Linux should not change it at runtime
>> +                */
> if we're not supposed to touch this the enable bit, can you switch to
> clk_regmap_gate_ro_ops ?

exact

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_fixed_factor g12a_sys_pll_div16 = {
>> +       .mult = 1,
>> +       .div = 16,
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "sys_pll_div16",
>> +               .ops = &clk_fixed_factor_ops,
>> +               .parent_names = (const char *[]){ "sys_pll_div16_en" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_dyn0_sel = {
>> +       .data = &(struct clk_regmap_mux_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
>> +               .mask = 0x3,
>> +               .shift = 0,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_dyn0_sel",
> the buildroot code has a variable with the name "p_premux"
> I'm not sure what the datasheet states, but maybe this should be
> cpu_clk_dyn0_pre_sel
> same applies to the corresponding dyn1 clock below

these bit are named "premux1", and cpu_clk_dyn0 names "postmux1",
which has no sense because there is no mux in between.

clk_dyn0_sel is the actual source selector of the dyn0 tree,
clkdyn0 is the top of the dyn0 tree, this is why i did not add "sel"
in it.

> 
>> +               .ops = &clk_regmap_mux_ro_ops,
>> +               .parent_names = (const char *[]){ IN_PREFIX "xtal",
>> +                                                 "fclk_div2",
>> +                                                 "fclk_div3" },
>> +               .num_parents = 3,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_dyn0_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
>> +               .shift = 4,
>> +               .width = 6,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_dyn0_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_dyn0_sel" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_dyn0 = {
>> +       .data = &(struct clk_regmap_mux_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
>> +               .mask = 0x1,
>> +               .shift = 2,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_dyn0",
> the buildroot code has a variable with the name "p_postmux". in this
> case I would leave the name "cpu_clk_dyn0" because it's the "output"
> of this specific "clock tree/branch".
> same applies to the corresponding dyn1 clock below
> 
>> +               .ops = &clk_regmap_mux_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_dyn0_sel",
>> +                                                 "cpu_clk_dyn0_div" },
>> +               .num_parents = 2,
>> +       },
>> +};
>> +

[...]

>> +
>> +static struct clk_regmap g12a_cpu_clk_div16_en = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 1,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_div16_en",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is used to debug the cpu_clk range
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_fixed_factor g12a_cpu_clk_div16 = {
>> +       .mult = 1,
>> +       .div = 16,
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_div16",
>> +               .ops = &clk_fixed_factor_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_div16_en" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_apb_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 3,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_apb_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_apb = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 1,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_apb",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_apb_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_atb_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 6,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_atb_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_atb = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 17,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_atb",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_atb_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_axi_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 9,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_axi_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
> out of curiosity (this applies to all CPU clock post-dividers):
> did you check whether CLK_DIVIDER_POWER_OF_TWO is correct on G12A?
> I'm asking because on Meson8b the post-dividers select between one of:
> "cpu_clk divided by 2, 3, 4, 5, 6, 7 or 8". also some of the
> post-dividers use register value 0 for cpu_clk_div2 while others use
> register value 1 for cpu_clk_div2.

It's correct !

> 
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_axi = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 18,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_axi",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_axi_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_trace_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 20,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_trace_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_trace = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 23,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_trace",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_trace_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>>  static const struct pll_mult_range g12a_gp0_pll_mult_range = {
>>         .min = 55,
>>         .max = 255,

Thanks !

Neil