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: <20190301102140.7181-1-narmstrong@baylibre.com>
 <20190301102140.7181-3-narmstrong@baylibre.com> <CAFBinCC1b5Q8Y=5qXKJ+B1XBhpo8PGFYHwTEeVTgbXwz7ge3-A@mail.gmail.com>
 <a3654bdb-4289-7755-5601-e8177b00790f@baylibre.com>
In-Reply-To: <a3654bdb-4289-7755-5601-e8177b00790f@baylibre.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Fri, 1 Mar 2019 17:52:34 +0100
Message-ID: <CAFBinCA6VJqK31NgyVkf2rLva0R2RbYQasdZGe+0BjjdOmnmFA@mail.gmail.com>
Subject: Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks
To:     Neil Armstrong <narmstrong@baylibre.com>
Cc:     jbrunet@baylibre.com, linux-amlogic@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Neil,

On Fri, Mar 1, 2019 at 5:42 PM Neil Armstrong <narmstrong@baylibre.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