Messages in this thread | | | From | Brendan Higgins <> | Date | Thu, 8 Feb 2018 16:07:44 -0800 | 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>
| |