linux-kernel - Re: [PATCH v6 04/41] clk: davinci: Add platform information for TI DA850 PLL

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, 2 Feb 2018 14:07:31 +0530
From:   Sekhar Nori <nsekhar@...com>
To:     David Lechner <david@...hnology.com>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
CC:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Kevin Hilman <khilman@...nel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Adam Ford <aford173@...il.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 04/41] clk: davinci: Add platform information for TI
 DA850 PLL

On Friday 02 February 2018 12:52 AM, David Lechner wrote:

>> +static const char * const da850_pll1_obsclk_parent_names[]
>> __initconst = {
>> +    "oscin",
> 
> Re: the issue of "ref_clk" vs. "oscin"...
> 
> This is one of the places where having the otherwise unnecessary "oscin"
> clock
> really helps out. The PLL driver doesn't control "ref_clk" - it comes
> from somewhere
> else. And in the case of DT, it may not even be named "ref_clk", so we
> really
> don't want to hard-code the name "ref_clk" here.

TBH, I don't really see what is wrong with mandating the name "ref_clk"
as the reference clock name to be provided. And for all board-files and
DTs to supply the same name.

> If we have to allow a variable name here, it just makes more work in the
> driver
> shuffling names around.
> 
> And the name "oscin" totally makes sense here because the TRM lists this
> input to the
> mux as "OSCIN".

Fine with me if you feel it simplifies implementation for you (and also
because of the distinction you want to make between the external "before
CLKMODE" clock and internal "after CLKMODE" clock). What I do care about
though is:

a) In the DT case, ability for different boards to provide different
   ref_clk frequencies. We never really had this in the legacy board
   file way (except some rudimentary support on DM6467T). And its fine
   to continue with status quo for board files.

b) In the DT case, ability for board to specify whether it uses the
   on-chip oscillator or has an external clean clock provider.

>> +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1)
>> +{
>> +    const struct davinci_pll_sysclk_info *info;
>> +
>> +    davinci_pll_clk_register(&da850_pll0_info, "ref_clk", pll0);
> 
> And really, we probably shouldn't be hard-coding "ref_clk" here either.
> Basically, we are making the assumption that the board file has registered
> a clock named "ref_clk". It would probably be better to pass the name
> as a parameter.

As I noted before, I am not sure if this level of naming flexibility is
needed. Every board needs to have one anyway. They might as well call it
by the same name.

That said, I wont oppose it either if you decide to have that flexibility.

Thanks,
Sekhar