linux-kernel - Re: [PATCH v1 2/2] clk: npcm: add NPCM7xx clock driver

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] [day] [month] [year] [list]

Date:   Thu, 8 Feb 2018 16:07:44 -0800
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Tali Perry <tali.perry1@...il.com>
Cc:      Tomer Maimon <tmaimon77@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        sboyd@...eaurora.org, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Avi Fishman <avifishman70@...il.com>,
        Joel Stanley <joel@....id.au>, linux-clk@...r.kernel.org,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH v1 2/2] clk: npcm: add NPCM7xx clock driver

<snip>
>> +
>> +       /* Define fixed clocks.
>> +        * Notice: the following clocks are fixed value on NPCM7XX and should
>> +        * not be changed.
>> +        * therefor they are not exposed to the dev tree .
>
> I am not convinced. The top level .dtsi is usually SoC specific.
>
> - CLKREF should be fixed on 25MHz. I didn't want to put it in DT since
> it might appear as something that is changeable.
>  on npcm750 all clocks are set in ROM +BB (several possible settings)
> , so this entire driver is only used for reading the current clock
> settings. All clocks are read only and derived from CLKREF. CLKREF is
> fixed so no point in putting it in DT and exposing it outside of this
> driver.

I understand your rationale, but the *.dtsi is typically used to describe the
SoC, things that cannot change. For example, consider
arch/arm/boot/dts/nuvoton-npcm750.dtsi, it describes the CPU cores, cache
hierarchy, peripherals and what busses they reside on, memory layout, etc. None
of these things are configurable; the *.dtsi just describes the SoC. Typically,
a *.dts is made for a particular board and imports a *.dtsi and does some
configuration, but it is not supposed to override things like what I listed
above (amoung other things).

There are several reasons that you put this information that cannot change in a
*.dtsi. For one it is a convenient, all in one place description of the SoC that
serves as a form of documentation. But more importantly, if you were to create a
variation of the 750 with a different number of cores, a different memory
layout, a different reference clock, etc, you could potentiallly use the same
drivers for both SoCs, and you would only have to write a new *.dtsi. I know you
may have no intention to do this, but many other CPU vendors do. And for this
reason, this is how drivers are expected to be written.

I hope this makes sense.

>
>> +        */
>> +       pr_debug("\tclk register fixed clocks\n");
>> +       hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_REFCLK,
>> +               NULL, 0, 25000000);
>> +       hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_SYSBYPCK,
>> +               NULL, 0, 800000000); // rarely used. mostly testing. TBD: remove
>> +       hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_MCBYPCK,
>> +               NULL, 0, 800000000); // rarely used.  mostly testing. TBD:remove
>> +
>> +
<snip>