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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Mar 2019 17:52:34 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Neil Armstrong <narmstrong@...libre.com>
Cc:     jbrunet@...libre.com, linux-amlogic@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks

Hi Neil,

On Fri, Mar 1, 2019 at 5:42 PM Neil Armstrong <narmstrong@...libre.com> wrote:
[...]
> >> +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.
OK, thank you for the explanation.
can you please add a comment with the name from the datasheet in that
case? that'll make it easier (at least for me) to compare the
datasheet (and also buildroot kernel, since similar names are used
there) with the mainline drivers

[...]
> >
> >> +               .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 !
thanks for looking this up again and double-checking!


Martin