Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755326AbbGZUcc (ORCPT ); Sun, 26 Jul 2015 16:32:32 -0400 Received: from fish.king.net.pl ([79.190.246.46]:49271 "EHLO king.net.pl" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754550AbbGZUc3 (ORCPT ); Sun, 26 Jul 2015 16:32:29 -0400 Date: Sun, 26 Jul 2015 22:24:08 +0200 (CEST) From: Paul Osmialowski X-X-Sender: newchief@localhost.localdomain To: Michael Turquette cc: Paul Osmialowski , Arnd Bergmann , Mark Rutland , Nicolas Pitre , Linus Walleij , Rob Herring , Alexander Potashev , Frank Li , Jiri Slaby , linux-clk@vger.kernel.org, Russell King , Vinod Koul , Geert Uytterhoeven , linux-serial@vger.kernel.org, Uwe Kleine-Koenig , Anson Huang , devicetree@vger.kernel.org, Pawel Moll , Ian Campbell , Kumar Gala , Yuri Tikhonov , linux-gpio@vger.kernel.org, Rob Herring , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Sergei Poselenov , Paul Bolle , Greg Kroah-Hartman , Stephen Boyd , linux-kernel@vger.kernel.org, Jingchang Lu , dmaengine@vger.kernel.org Subject: Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC In-Reply-To: <20150724034229.642.88156@quantum> Message-ID: References: <1435667250-28299-1-git-send-email-pawelo@king.net.pl> <2009463.YLtdMegFel@wuerfel> <20150724034229.642.88156@quantum> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9088 Lines: 277 Hi Mike, Thank you for spending time on this and pointing me into the right direction. I'm wondering about going even further with it. Assuming that I know everything about my board, I can skip run-time discovery phase (note that the original driver was designed for other Kinetis-based boards too) and move everything into DTS, somewhat like this: / { osc0: clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <50000000>; }; osc1: clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <12000000>; }; rtc: clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>; }; mcgout: clock { compatible = "fixed-factor-clock"; #clock-cells = <0>; clocks = <&osc0>; clock-mult = <12>; clock-div = <5>; }; core: clock { compatible = "fixed-factor-clock"; #clock-cells = <0>; clocks = <&mcgout>; clock-mult = <1>; clock-div = <1>; }; bus: clock { compatible = "fixed-factor-clock"; #clock-cells = <0>; clocks = <&mcgout>; clock-mult = <1>; clock-div = <2>; }; soc { cmu@0x40047000 { compatible = "fsl,kinetis-gate-clock"; reg = <0x40047000 0x1100>; mcg_core_gate: clock-gate { clocks = <&core>; #clock-cells = <2>; }; mcg_bus_gate: clock-gate { clocks = <&bus>; #clock-cells = <2>; }; osc0_erclk_gate: clock-gate { clocks = <&osc0>; #clock-cells = <2>; }; }; uart0: serial@4006a000 { compatible = "fsl,kinetis-lpuart"; reg = <0x4006a000 0x1000>; interrupts = <45>, <46>; interrupt-names = "uart-stat", "uart-err"; clocks = <&mcg_core_gate 3 10>; clock-names = "ipg"; dmas = <&edma 0 2>; dma-names = "rx"; status = "disabled"; }; }; }; As you can see, mcg part is not required anymore. I guess that the approach above would require split into soc-specific and board-specific part (as I said, dividers arrangement is something board specific), but I wonder what you thing about this proposal. Thanks, Paul On Thu, 23 Jul 2015, Michael Turquette wrote: > Quoting Paul Osmialowski (2015-07-04 14:50:03) > > Hi Arnd, > > > > I'm attaching excerpt from Kinetis reference manual that may make > > situation clearer. > > Hi Paul, > > Can you please post the patch in the body of the email instead of an > attachment? It makes it easier to review. Another small nitpick is that > the $SUBJECT for this patch might be better off as something like: > > clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC > > At least it helps me find the patch I care about when skimming the > series ;-) > > > > > These MCG and SIM registers are used only to determine configuration > > (clock fixed rates and clock signal origins) at run time. > > > > Namely, the real MCGOUTCLK source (in the middle) which is the parent for > > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by > > reading MCG registers, let me quote commit message from Emcraft git repo: > > > > * Determine in run-time what oscillator module (OSC0 or OSC1) is used > > as clock source for the main PLL. > > According to [0] there are three options: a 32k RTC osc clock and osc0 > both feed into a mux. You should model this 32k clock with the > fixed-rate binding. > > > * When OSC1 is selected, assume its frequency to be 12 MHz on all > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and > > TWR-K70F120M boards). > > > > In my .dts I'm trying to possibly follow real clock hierarchy, but to go > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by > > U-boot. But that's too demanding for any potential users of this BSP. So > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and > > PCLK. > > I'm confused. The point of device tree is to solve problems like this; > i.e. board-specific differences such as different oscillator > frequencies. > > OSC0 and OSC1 should each be a fixed-rate clock in your board-specific > TWR-K70F120M DTS (not a chip-specific file). They do not belong in the > cmu node, and they should use the "fixed-clock" binding. The 32k RTC osc > can probably go in your chip-specific .dtsi as a fixed-rate clock since > it appears to mandated in the reference manual[0]. > > These three fixed-rate clocks are your root clock nodes. Customers only > need to worry about this if they spin a board, and then they will need > to populate the frequencies of OSC0 and OSC1 in their board-specific > .dts. > > Please break clk-kinetis.c into two files: > drivers/clk/kinetis/clk-mcg.c > drivers/clk/kinetis/clk-sim.c > > Below is what your binding/dts should look like: > > { > osc0: clock { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <50000000>; > }; > > osc1: clock { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <12000000>; > }; > > rtc: clock { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <32768>; > }; > > soc: soc { > mcg: clock-controller@40064000 { > compatible = "fsl,kinetis-mcg"; > clock-cells = <1>; > reg = <0x40064000 0x14>; > clocks = <&osc0>, <&osc1>, <&rtc>; > clock-names = "osc0", "osc1", "rtc"; > }; > > sim: clock-controller@40047000 { > compatible = "fsl,kinetis-sim"; > clock-cells = <1>; > reg = <0x40047000 0x1100>; > clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; > clock-names = "core", "bus", "flexbus", "flash"; > }; > }; > > uart0: serial@4006a000 { > compatible = "fsl,kinetis-lpuart"; > reg = <0x4006a000 0x1000>; > clocks = <&sim SIM_SCGC4_UART1_CLK>; > clock-names = "gate"; > }; > > I removed the interrupts and dma stuff from the uart0 node for clarity. > The above is the only style of binding that I have been accepting for > some time; first declare the clock controller and establish its register > space, and then consumers can consume clocks by providing the phandle to > the controller plus an offset corresponding to a unique clock. The > clock-names property makes it really easy to use with the clkdev stuff > (e.g. clk_get()). > > I've covered this before on the mailing list so here is a link > describing how the qcom bindings do it in detail: > > http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum> > > Technically you could encode the same bits as sub-nodes of the mcg and > sim nodes, but the shared header is how the magic happens with the > driver so it's best to keep the clock controller binding small and > light. > > I think this means you can also get rid of kinetis_of_clk_get_name and > kinetis_clk_gate_get but my brain is tired so I'll leave that as an > exercise to the reader. > > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf > > Regards, > Mike > > > > > In my most recent version I added OSC0ERCLK explicitly as one more root > > clock, since it is also used directly (through CG reg. 1 bit 0) by > > Freescale fec network device whose in-tree driver I'm trying to make > > usable for Kinetis. > > > > On Sat, 4 Jul 2015, Arnd Bergmann wrote: > > > > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote: > > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote: > > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote: > > >>> > > >>>> I wonder if you could move out the fixed rate clocks into their own > > >>>> nodes. Are they actually controlled by the same block? If they are > > >>>> just fixed, you can use the normal binding for fixed rate clocks > > >>>> and only describe the clocks that are related to the driver. > > >>> > > >>> In my view having these clocks grouped together looks more convincing. After > > >>> all, they all share the same I/O regs in order to read configuration. > > >> > > >> The fact that they share a register is not making them a group. That's > > >> just a HW design decision and you need to deal with that by protecting > > >> the register access, but not by trying to group them artificially at > > >> the functional level. > > > > > > I'd disagree with that: The clock controller is the device that owns the > > > registers and that should be one node in DT, as Paul's first version does. > > > > > > The part I'm still struggling with is understanding how the fixed-rate > > > clocks are controlled through those registers. If they are indeed configured > > > through the registers, the name is probably wrong and should be changed > > > to whatever kind of non-fixed clock this is. > > > > > > Arnd > > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/