diff for duplicates of <20150728160347.642.31950@quantum> diff --git a/a/1.txt b/N1/1.txt index bd0ccbc..2783a71 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,52 +1,42 @@ Quoting Paul Osmialowski (2015-07-26 13:24:08) > 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 = - +> +> 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 Hi Paul, No problem! And thanks for the quick turnaround on your patches so far. -> 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) = - +> 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 =3D "fixed-clock"; -> #clock-cells =3D <0>; -> clock-frequency =3D <50000000>; +> compatible = "fixed-clock"; +> #clock-cells = <0>; +> clock-frequency = <50000000>; > }; -> = - +> > osc1: clock { -> compatible =3D "fixed-clock"; -> #clock-cells =3D <0>; -> clock-frequency =3D <12000000>; +> compatible = "fixed-clock"; +> #clock-cells = <0>; +> clock-frequency = <12000000>; > }; -> = - +> > rtc: clock { -> compatible =3D "fixed-clock"; -> #clock-cells =3D <0>; -> clock-frequency =3D <32768>; +> compatible = "fixed-clock"; +> #clock-cells = <0>; +> clock-frequency = <32768>; > }; -> = - +> > mcgout: clock { -> compatible =3D "fixed-factor-clock"; -> #clock-cells =3D <0>; -> clocks =3D <&osc0>; -> clock-mult =3D <12>; -> clock-div =3D <5>; +> compatible = "fixed-factor-clock"; +> #clock-cells = <0>; +> clocks = <&osc0>; +> clock-mult = <12>; +> clock-div = <5>; > }; I think this is a step backwards. @@ -85,23 +75,21 @@ So why is it listed here as a fixed-factor clock? Is it not a multiplexer? Also, why is it listed here at all? Please take another look at the qcom binding example I linked to in my previous mail. -> = - +> > core: clock { -> compatible =3D "fixed-factor-clock"; -> #clock-cells =3D <0>; -> clocks =3D <&mcgout>; -> clock-mult =3D <1>; -> clock-div =3D <1>; +> compatible = "fixed-factor-clock"; +> #clock-cells = <0>; +> clocks = <&mcgout>; +> clock-mult = <1>; +> clock-div = <1>; > }; -> = - +> > bus: clock { -> compatible =3D "fixed-factor-clock"; -> #clock-cells =3D <0>; -> clocks =3D <&mcgout>; -> clock-mult =3D <1>; -> clock-div =3D <2>; +> compatible = "fixed-factor-clock"; +> #clock-cells = <0>; +> clocks = <&mcgout>; +> clock-mult = <1>; +> clock-div = <2>; These are actually not fixed dividers but programmable dividers. You can probably use drivers/clk/clk-divider.c for these. I'm fine with using @@ -121,54 +109,48 @@ property of some device which *consumes* the clock, not the clock controller or the clock output itself. > }; -> = - +> > soc { > cmu@0x40047000 { -> compatible =3D "fsl,kinetis-gate-clock"; -> reg =3D <0x40047000 0x1100>; -> = - +> compatible = "fsl,kinetis-gate-clock"; +> reg = <0x40047000 0x1100>; +> > mcg_core_gate: clock-gate { -> clocks =3D <&core>; -> #clock-cells =3D <2>; +> clocks = <&core>; +> #clock-cells = <2>; > }; -> = - +> > mcg_bus_gate: clock-gate { -> clocks =3D <&bus>; -> #clock-cells =3D <2>; +> clocks = <&bus>; +> #clock-cells = <2>; > }; -> = - +> > osc0_erclk_gate: clock-gate { -> clocks =3D <&osc0>; -> #clock-cells =3D <2>; +> clocks = <&osc0>; +> #clock-cells = <2>; > }; > }; -> = - +> > uart0: serial@4006a000 { -> compatible =3D "fsl,kinetis-lpuart"; -> reg =3D <0x4006a000 0x1000>; -> interrupts =3D <45>, <46>; -> interrupt-names =3D "uart-stat", "uart-err"; -> clocks =3D <&mcg_core_gate 3 10>; +> compatible = "fsl,kinetis-lpuart"; +> reg = <0x4006a000 0x1000>; +> interrupts = <45>, <46>; +> interrupt-names = "uart-stat", "uart-err"; +> clocks = <&mcg_core_gate 3 10>; Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example. -> clock-names =3D "ipg"; -> dmas =3D <&edma 0 2>; -> dma-names =3D "rx"; -> status =3D "disabled"; +> clock-names = "ipg"; +> dmas = <&edma 0 2>; +> dma-names = "rx"; +> status = "disabled"; > }; > }; > }; -> = - +> > As you can see, mcg part is not required anymore. I think the mcg should be required. The mcg is a real IP block on your @@ -179,12 +161,9 @@ that you should. I did a quick grep and didn't find "cmu" anywhere in the reference manual. -> = - -> I guess that the approach above would require split into soc-specific and = - -> board-specific part (as I said, dividers arrangement is something board = - +> +> 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. Splitting is good. Chip-specific stuff can go into the chip-specific @@ -194,165 +173,119 @@ files. The ultimate goal is to make it trivial to add new boards. Regards, Mike -> = - +> > 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 = - +> > > +> > > 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 = - +> > +> > > +> > > 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 ti= -me by = - -> > > reading MCG registers, let me quote commit message from Emcraft git r= -epo: -> > > = - -> > > * Determine in run-time what oscillator module (OSC0 or OSC1) i= -s used +> > > +> > > 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 a= -ll -> > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM = -and +> > +> > > * 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 an= -d = - +> > > +> > > 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 =3D "fixed-clock"; -> > #clock-cells =3D <0>; -> > clock-frequency =3D <50000000>; +> > compatible = "fixed-clock"; +> > #clock-cells = <0>; +> > clock-frequency = <50000000>; > > }; -> > = - +> > > > osc1: clock { -> > compatible =3D "fixed-clock"; -> > #clock-cells =3D <0>; -> > clock-frequency =3D <12000000>; +> > compatible = "fixed-clock"; +> > #clock-cells = <0>; +> > clock-frequency = <12000000>; > > }; -> > = - +> > > > rtc: clock { -> > compatible =3D "fixed-clock"; -> > #clock-cells =3D <0>; -> > clock-frequency =3D <32768>; +> > compatible = "fixed-clock"; +> > #clock-cells = <0>; +> > clock-frequency = <32768>; > > }; -> > = - +> > > > soc: soc { > > mcg: clock-controller@40064000 { -> > compatible =3D "fsl,kinetis-mcg"; -> > clock-cells =3D <1>; -> > reg =3D <0x40064000 0x14>; -> > clocks =3D <&osc0>, <&osc1>, <&rtc>; -> > clock-names =3D "osc0", "osc1", "rtc"; +> > compatible = "fsl,kinetis-mcg"; +> > clock-cells = <1>; +> > reg = <0x40064000 0x14>; +> > clocks = <&osc0>, <&osc1>, <&rtc>; +> > clock-names = "osc0", "osc1", "rtc"; > > }; -> > = - +> > > > sim: clock-controller@40047000 { -> > compatible =3D "fsl,kinetis-sim"; -> > clock-cells =3D <1>; -> > reg =3D <0x40047000 0x1100>; -> > clocks =3D <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_M= -CGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; -> > clock-names =3D "core", "bus", "flexbus", "flash"; +> > 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 =3D "fsl,kinetis-lpuart"; -> > reg =3D <0x4006a000 0x1000>; -> > clocks =3D <&sim SIM_SCGC4_UART1_CLK>; -> > clock-names =3D "gate"; +> > 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 @@ -360,96 +293,67 @@ CGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; > > 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/K7= -0P256M150SF3RM.pdf -> > = - +> > +> > [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 ro= -ot = - -> > > 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 = - +> > +> > > +> > > 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 a= -re +> > > >>>> 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 convin= -cing. After -> > > >>> all, they all share the same I/O regs in order to read configurat= -ion. +> > > >>> 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. Th= -at's -> > > >> just a HW design decision and you need to deal with that by protec= -ting -> > > >> the register access, but not by trying to group them artificially = -at +> > > >> 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 own= -s the -> > > > registers and that should be one node in DT, as Paul's first versio= -n does. +> > > > 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-r= -ate -> > > > clocks are controlled through those registers. If they are indeed c= -onfigured -> > > > through the registers, the name is probably wrong and should be cha= -nged +> > > > 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 diff --git a/a/content_digest b/N1/content_digest index ad41a26..e61eb72 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -8,11 +8,7 @@ "From\0Michael Turquette <mturquette@baylibre.com>\0" "Subject\0Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC\0" "Date\0Tue, 28 Jul 2015 09:03:47 -0700\0" - "To\0Paul Osmialowski <pawelo@king.net.pl>" - "\0" - "Cc\0Paul Osmialowski <pawelo@king.net.pl>" - Arnd Bergmann <arnd@arndb.de> - Mark Rutland <mark.rutland@arm.com> + "Cc\0Mark Rutland <mark.rutland@arm.com>" Nicolas Pitre <nicolas.pitre@linaro.org> Linus Walleij <linus.walleij@linaro.org> Rob Herring <r.herring@freescale.com> @@ -21,78 +17,62 @@ Jiri Slaby <jslaby@suse.cz> linux-clk@vger.kernel.org Russell King <linux@arm.linux.org.uk> + Arnd Bergmann <arnd@arndb.de> Vinod Koul <vinod.koul@intel.com> Geert Uytterhoeven <geert@linux-m68k.org> linux-serial@vger.kernel.org Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de> Anson Huang <b20788@freescale.com> devicetree@vger.kernel.org + Paul Osmialowski <pawelo@king.net.pl> Pawel Moll <pawel.moll@arm.com> Ian Campbell <ijc+devicetree@hellion.org.uk> - Kumar Gala <galak@codeaurora.org> + Jingchang Lu <jingchang.lu@freescale.com> Yuri Tikhonov <yur@emcraft.com> linux-gpio@vger.kernel.org Rob Herring <robh+dt@kernel.org> - Thomas Gleixner <tglx@linutronix.de> - linux-arm-kernel@lists.infradead.org - Sergei Poselenov <sposelenov@emcraft.com> - Paul Bolle <pebolle@tiscali.nl> - Greg Kroah-Hartman <gregkh@linuxfoundation.org> - Stephen Boyd <sboyd@codeaurora.org> - linux-kernel@vger.kernel.org - Jingchang Lu <jingchang.lu@freescale.com> - " dmaengine@vger.kernel.org\0" + " Thomas Gleixner <tglx@linutronix.de>\0" "\00:1\0" "b\0" "Quoting Paul Osmialowski (2015-07-26 13:24:08)\n" "> Hi Mike,\n" - "> =\n" - "\n" - "> Thank you for spending time on this and pointing me into the right =\n" - "\n" - "> direction. I'm wondering about going even further with it. Assuming that =\n" - "I =\n" - "\n" + "> \n" + "> Thank you for spending time on this and pointing me into the right \n" + "> direction. I'm wondering about going even further with it. Assuming that I \n" "\n" "Hi Paul,\n" "\n" "No problem! And thanks for the quick turnaround on your patches so far.\n" "\n" - "> know everything about my board, I can skip run-time discovery phase (note =\n" - "\n" - "> that the original driver was designed for other Kinetis-based boards too) =\n" - "\n" + "> know everything about my board, I can skip run-time discovery phase (note \n" + "> that the original driver was designed for other Kinetis-based boards too) \n" "> and move everything into DTS, somewhat like this:\n" - "> =\n" - "\n" + "> \n" "> / {\n" "> osc0: clock {\n" - "> compatible =3D \"fixed-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clock-frequency =3D <50000000>;\n" + "> compatible = \"fixed-clock\";\n" + "> #clock-cells = <0>;\n" + "> clock-frequency = <50000000>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> osc1: clock {\n" - "> compatible =3D \"fixed-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clock-frequency =3D <12000000>;\n" + "> compatible = \"fixed-clock\";\n" + "> #clock-cells = <0>;\n" + "> clock-frequency = <12000000>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> rtc: clock {\n" - "> compatible =3D \"fixed-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clock-frequency =3D <32768>;\n" + "> compatible = \"fixed-clock\";\n" + "> #clock-cells = <0>;\n" + "> clock-frequency = <32768>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> mcgout: clock {\n" - "> compatible =3D \"fixed-factor-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clocks =3D <&osc0>;\n" - "> clock-mult =3D <12>;\n" - "> clock-div =3D <5>;\n" + "> compatible = \"fixed-factor-clock\";\n" + "> #clock-cells = <0>;\n" + "> clocks = <&osc0>;\n" + "> clock-mult = <12>;\n" + "> clock-div = <5>;\n" "> };\n" "\n" "I think this is a step backwards.\n" @@ -131,23 +111,21 @@ "multiplexer? Also, why is it listed here at all? Please take another\n" "look at the qcom binding example I linked to in my previous mail.\n" "\n" - "> =\n" - "\n" + "> \n" "> core: clock {\n" - "> compatible =3D \"fixed-factor-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clocks =3D <&mcgout>;\n" - "> clock-mult =3D <1>;\n" - "> clock-div =3D <1>;\n" + "> compatible = \"fixed-factor-clock\";\n" + "> #clock-cells = <0>;\n" + "> clocks = <&mcgout>;\n" + "> clock-mult = <1>;\n" + "> clock-div = <1>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> bus: clock {\n" - "> compatible =3D \"fixed-factor-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clocks =3D <&mcgout>;\n" - "> clock-mult =3D <1>;\n" - "> clock-div =3D <2>;\n" + "> compatible = \"fixed-factor-clock\";\n" + "> #clock-cells = <0>;\n" + "> clocks = <&mcgout>;\n" + "> clock-mult = <1>;\n" + "> clock-div = <2>;\n" "\n" "These are actually not fixed dividers but programmable dividers. You can\n" "probably use drivers/clk/clk-divider.c for these. I'm fine with using\n" @@ -167,54 +145,48 @@ "controller or the clock output itself.\n" "\n" "> };\n" - "> =\n" - "\n" + "> \n" "> soc {\n" "> cmu@0x40047000 {\n" - "> compatible =3D \"fsl,kinetis-gate-clock\";\n" - "> reg =3D <0x40047000 0x1100>;\n" - "> =\n" - "\n" + "> compatible = \"fsl,kinetis-gate-clock\";\n" + "> reg = <0x40047000 0x1100>;\n" + "> \n" "> mcg_core_gate: clock-gate {\n" - "> clocks =3D <&core>;\n" - "> #clock-cells =3D <2>;\n" + "> clocks = <&core>;\n" + "> #clock-cells = <2>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> mcg_bus_gate: clock-gate {\n" - "> clocks =3D <&bus>;\n" - "> #clock-cells =3D <2>;\n" + "> clocks = <&bus>;\n" + "> #clock-cells = <2>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> osc0_erclk_gate: clock-gate {\n" - "> clocks =3D <&osc0>;\n" - "> #clock-cells =3D <2>;\n" + "> clocks = <&osc0>;\n" + "> #clock-cells = <2>;\n" "> };\n" "> };\n" - "> =\n" - "\n" + "> \n" "> uart0: serial@4006a000 {\n" - "> compatible =3D \"fsl,kinetis-lpuart\";\n" - "> reg =3D <0x4006a000 0x1000>;\n" - "> interrupts =3D <45>, <46>;\n" - "> interrupt-names =3D \"uart-stat\", \"uart-err\";\n" - "> clocks =3D <&mcg_core_gate 3 10>;\n" + "> compatible = \"fsl,kinetis-lpuart\";\n" + "> reg = <0x4006a000 0x1000>;\n" + "> interrupts = <45>, <46>;\n" + "> interrupt-names = \"uart-stat\", \"uart-err\";\n" + "> clocks = <&mcg_core_gate 3 10>;\n" "\n" "Magic numbers are not good. dtc has been able to use preprocessor macros\n" "for a while now which means we can use constants instead of magic\n" "numbers. Please look at the shared header in the qcom binding for an\n" "example.\n" "\n" - "> clock-names =3D \"ipg\";\n" - "> dmas =3D <&edma 0 2>;\n" - "> dma-names =3D \"rx\";\n" - "> status =3D \"disabled\";\n" + "> clock-names = \"ipg\";\n" + "> dmas = <&edma 0 2>;\n" + "> dma-names = \"rx\";\n" + "> status = \"disabled\";\n" "> };\n" "> };\n" "> };\n" - "> =\n" - "\n" + "> \n" "> As you can see, mcg part is not required anymore.\n" "\n" "I think the mcg should be required. The mcg is a real IP block on your\n" @@ -225,12 +197,9 @@ "I did a quick grep and didn't find \"cmu\" anywhere in the reference\n" "manual.\n" "\n" - "> =\n" - "\n" - "> I guess that the approach above would require split into soc-specific and =\n" - "\n" - "> board-specific part (as I said, dividers arrangement is something board =\n" - "\n" + "> \n" + "> I guess that the approach above would require split into soc-specific and \n" + "> board-specific part (as I said, dividers arrangement is something board \n" "> specific), but I wonder what you thing about this proposal.\n" "\n" "Splitting is good. Chip-specific stuff can go into the chip-specific\n" @@ -240,165 +209,119 @@ "Regards,\n" "Mike\n" "\n" - "> =\n" - "\n" + "> \n" "> Thanks,\n" "> Paul\n" - "> =\n" - "\n" + "> \n" "> On Thu, 23 Jul 2015, Michael Turquette wrote:\n" - "> =\n" - "\n" + "> \n" "> > Quoting Paul Osmialowski (2015-07-04 14:50:03)\n" "> > > Hi Arnd,\n" - "> > > =\n" - "\n" - "> > > I'm attaching excerpt from Kinetis reference manual that may make =\n" - "\n" + "> > > \n" + "> > > I'm attaching excerpt from Kinetis reference manual that may make \n" "> > > situation clearer.\n" - "> > =\n" - "\n" + "> > \n" "> > Hi Paul,\n" - "> > =\n" - "\n" + "> > \n" "> > Can you please post the patch in the body of the email instead of an\n" "> > attachment? It makes it easier to review. Another small nitpick is that\n" "> > the $SUBJECT for this patch might be better off as something like:\n" - "> > =\n" - "\n" + "> > \n" "> > clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC\n" - "> > =\n" - "\n" + "> > \n" "> > At least it helps me find the patch I care about when skimming the\n" "> > series ;-)\n" - "> > =\n" - "\n" - "> > > =\n" - "\n" - "> > > These MCG and SIM registers are used only to determine configuration =\n" - "\n" + "> > \n" + "> > > \n" + "> > > These MCG and SIM registers are used only to determine configuration \n" "> > > (clock fixed rates and clock signal origins) at run time.\n" - "> > > =\n" - "\n" - "> > > Namely, the real MCGOUTCLK source (in the middle) which is the parent=\n" - " for =\n" - "\n" - "> > > core clock (CCLK) and peripheral clock (PCLK) is determined at run ti=\n" - "me by =\n" - "\n" - "> > > reading MCG registers, let me quote commit message from Emcraft git r=\n" - "epo:\n" - "> > > =\n" - "\n" - "> > > * Determine in run-time what oscillator module (OSC0 or OSC1) i=\n" - "s used\n" + "> > > \n" + "> > > Namely, the real MCGOUTCLK source (in the middle) which is the parent for \n" + "> > > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by \n" + "> > > reading MCG registers, let me quote commit message from Emcraft git repo:\n" + "> > > \n" + "> > > * Determine in run-time what oscillator module (OSC0 or OSC1) is used\n" "> > > as clock source for the main PLL.\n" - "> > =\n" - "\n" + "> > \n" "> > According to [0] there are three options: a 32k RTC osc clock and osc0\n" "> > both feed into a mux. You should model this 32k clock with the\n" "> > fixed-rate binding.\n" - "> > =\n" - "\n" - "> > > * When OSC1 is selected, assume its frequency to be 12 MHz on a=\n" - "ll\n" - "> > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM =\n" - "and\n" + "> > \n" + "> > > * When OSC1 is selected, assume its frequency to be 12 MHz on all\n" + "> > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and\n" "> > > TWR-K70F120M boards).\n" - "> > > =\n" - "\n" - "> > > In my .dts I'm trying to possibly follow real clock hierarchy, but to=\n" - " go =\n" - "\n" - "> > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. =\n" - "by =\n" - "\n" - "> > > U-boot. But that's too demanding for any potential users of this BSP.=\n" - " So =\n" - "\n" - "> > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK an=\n" - "d =\n" - "\n" + "> > > \n" + "> > > In my .dts I'm trying to possibly follow real clock hierarchy, but to go \n" + "> > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by \n" + "> > > U-boot. But that's too demanding for any potential users of this BSP. So \n" + "> > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and \n" "> > > PCLK.\n" - "> > =\n" - "\n" + "> > \n" "> > I'm confused. The point of device tree is to solve problems like this;\n" "> > i.e. board-specific differences such as different oscillator\n" "> > frequencies.\n" - "> > =\n" - "\n" + "> > \n" "> > OSC0 and OSC1 should each be a fixed-rate clock in your board-specific\n" "> > TWR-K70F120M DTS (not a chip-specific file). They do not belong in the\n" "> > cmu node, and they should use the \"fixed-clock\" binding. The 32k RTC osc\n" "> > can probably go in your chip-specific .dtsi as a fixed-rate clock since\n" "> > it appears to mandated in the reference manual[0].\n" - "> > =\n" - "\n" + "> > \n" "> > These three fixed-rate clocks are your root clock nodes. Customers only\n" "> > need to worry about this if they spin a board, and then they will need\n" "> > to populate the frequencies of OSC0 and OSC1 in their board-specific\n" "> > .dts.\n" - "> > =\n" - "\n" + "> > \n" "> > Please break clk-kinetis.c into two files:\n" "> > drivers/clk/kinetis/clk-mcg.c\n" "> > drivers/clk/kinetis/clk-sim.c\n" - "> > =\n" - "\n" + "> > \n" "> > Below is what your binding/dts should look like:\n" - "> > =\n" - "\n" + "> > \n" "> > {\n" "> > osc0: clock {\n" - "> > compatible =3D \"fixed-clock\";\n" - "> > #clock-cells =3D <0>;\n" - "> > clock-frequency =3D <50000000>;\n" + "> > compatible = \"fixed-clock\";\n" + "> > #clock-cells = <0>;\n" + "> > clock-frequency = <50000000>;\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > osc1: clock {\n" - "> > compatible =3D \"fixed-clock\";\n" - "> > #clock-cells =3D <0>;\n" - "> > clock-frequency =3D <12000000>;\n" + "> > compatible = \"fixed-clock\";\n" + "> > #clock-cells = <0>;\n" + "> > clock-frequency = <12000000>;\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > rtc: clock {\n" - "> > compatible =3D \"fixed-clock\";\n" - "> > #clock-cells =3D <0>;\n" - "> > clock-frequency =3D <32768>;\n" + "> > compatible = \"fixed-clock\";\n" + "> > #clock-cells = <0>;\n" + "> > clock-frequency = <32768>;\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > soc: soc {\n" "> > mcg: clock-controller@40064000 {\n" - "> > compatible =3D \"fsl,kinetis-mcg\";\n" - "> > clock-cells =3D <1>;\n" - "> > reg =3D <0x40064000 0x14>;\n" - "> > clocks =3D <&osc0>, <&osc1>, <&rtc>;\n" - "> > clock-names =3D \"osc0\", \"osc1\", \"rtc\";\n" + "> > compatible = \"fsl,kinetis-mcg\";\n" + "> > clock-cells = <1>;\n" + "> > reg = <0x40064000 0x14>;\n" + "> > clocks = <&osc0>, <&osc1>, <&rtc>;\n" + "> > clock-names = \"osc0\", \"osc1\", \"rtc\";\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > sim: clock-controller@40047000 {\n" - "> > compatible =3D \"fsl,kinetis-sim\";\n" - "> > clock-cells =3D <1>;\n" - "> > reg =3D <0x40047000 0x1100>;\n" - "> > clocks =3D <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_M=\n" - "CGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;\n" - "> > clock-names =3D \"core\", \"bus\", \"flexbus\", \"flash\";\n" + "> > compatible = \"fsl,kinetis-sim\";\n" + "> > clock-cells = <1>;\n" + "> > reg = <0x40047000 0x1100>;\n" + "> > clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;\n" + "> > clock-names = \"core\", \"bus\", \"flexbus\", \"flash\";\n" "> > };\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > uart0: serial@4006a000 {\n" - "> > compatible =3D \"fsl,kinetis-lpuart\";\n" - "> > reg =3D <0x4006a000 0x1000>;\n" - "> > clocks =3D <&sim SIM_SCGC4_UART1_CLK>;\n" - "> > clock-names =3D \"gate\";\n" + "> > compatible = \"fsl,kinetis-lpuart\";\n" + "> > reg = <0x4006a000 0x1000>;\n" + "> > clocks = <&sim SIM_SCGC4_UART1_CLK>;\n" + "> > clock-names = \"gate\";\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > I removed the interrupts and dma stuff from the uart0 node for clarity.\n" "> > The above is the only style of binding that I have been accepting for\n" "> > some time; first declare the clock controller and establish its register\n" @@ -406,100 +329,71 @@ "> > the controller plus an offset corresponding to a unique clock. The\n" "> > clock-names property makes it really easy to use with the clkdev stuff\n" "> > (e.g. clk_get()).\n" - "> > =\n" - "\n" + "> > \n" "> > I've covered this before on the mailing list so here is a link\n" "> > describing how the qcom bindings do it in detail:\n" - "> > =\n" - "\n" + "> > \n" "> > http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum>\n" - "> > =\n" - "\n" + "> > \n" "> > Technically you could encode the same bits as sub-nodes of the mcg and\n" "> > sim nodes, but the shared header is how the magic happens with the\n" "> > driver so it's best to keep the clock controller binding small and\n" "> > light.\n" - "> > =\n" - "\n" + "> > \n" "> > I think this means you can also get rid of kinetis_of_clk_get_name and\n" "> > kinetis_clk_gate_get but my brain is tired so I'll leave that as an\n" "> > exercise to the reader.\n" - "> > =\n" - "\n" - "> > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K7=\n" - "0P256M150SF3RM.pdf\n" - "> > =\n" - "\n" + "> > \n" + "> > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf\n" + "> > \n" "> > Regards,\n" "> > Mike\n" - "> > =\n" - "\n" - "> > > =\n" - "\n" - "> > > In my most recent version I added OSC0ERCLK explicitly as one more ro=\n" - "ot =\n" - "\n" - "> > > clock, since it is also used directly (through CG reg. 1 bit 0) by =\n" - "\n" - "> > > Freescale fec network device whose in-tree driver I'm trying to make =\n" - "\n" + "> > \n" + "> > > \n" + "> > > In my most recent version I added OSC0ERCLK explicitly as one more root \n" + "> > > clock, since it is also used directly (through CG reg. 1 bit 0) by \n" + "> > > Freescale fec network device whose in-tree driver I'm trying to make \n" "> > > usable for Kinetis.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > On Sat, 4 Jul 2015, Arnd Bergmann wrote:\n" - "> > > =\n" - "\n" + "> > > \n" "> > > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:\n" "> > > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote:\n" "> > > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote:\n" "> > > >>>\n" - "> > > >>>> I wonder if you could move out the fixed rate clocks into their =\n" - "own\n" - "> > > >>>> nodes. Are they actually controlled by the same block? If they a=\n" - "re\n" + "> > > >>>> I wonder if you could move out the fixed rate clocks into their own\n" + "> > > >>>> nodes. Are they actually controlled by the same block? If they are\n" "> > > >>>> just fixed, you can use the normal binding for fixed rate clocks\n" "> > > >>>> and only describe the clocks that are related to the driver.\n" "> > > >>>\n" - "> > > >>> In my view having these clocks grouped together looks more convin=\n" - "cing. After\n" - "> > > >>> all, they all share the same I/O regs in order to read configurat=\n" - "ion.\n" + "> > > >>> In my view having these clocks grouped together looks more convincing. After\n" + "> > > >>> all, they all share the same I/O regs in order to read configuration.\n" "> > > >>\n" - "> > > >> The fact that they share a register is not making them a group. Th=\n" - "at's\n" - "> > > >> just a HW design decision and you need to deal with that by protec=\n" - "ting\n" - "> > > >> the register access, but not by trying to group them artificially =\n" - "at\n" + "> > > >> The fact that they share a register is not making them a group. That's\n" + "> > > >> just a HW design decision and you need to deal with that by protecting\n" + "> > > >> the register access, but not by trying to group them artificially at\n" "> > > >> the functional level.\n" "> > > >\n" - "> > > > I'd disagree with that: The clock controller is the device that own=\n" - "s the\n" - "> > > > registers and that should be one node in DT, as Paul's first versio=\n" - "n does.\n" + "> > > > I'd disagree with that: The clock controller is the device that owns the\n" + "> > > > registers and that should be one node in DT, as Paul's first version does.\n" "> > > >\n" - "> > > > The part I'm still struggling with is understanding how the fixed-r=\n" - "ate\n" - "> > > > clocks are controlled through those registers. If they are indeed c=\n" - "onfigured\n" - "> > > > through the registers, the name is probably wrong and should be cha=\n" - "nged\n" + "> > > > The part I'm still struggling with is understanding how the fixed-rate\n" + "> > > > clocks are controlled through those registers. If they are indeed configured\n" + "> > > > through the registers, the name is probably wrong and should be changed\n" "> > > > to whatever kind of non-fixed clock this is.\n" "> > > >\n" "> > > > Arnd\n" "> > > >\n" - "> > > =\n" - "\n" + "> > > \n" "> > > _______________________________________________\n" "> > > linux-arm-kernel mailing list\n" "> > > linux-arm-kernel@lists.infradead.org\n" "> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel\n" - "> > =\n" - "\n" + "> > \n" "> --\n" "> To unsubscribe from this list: send the line \"unsubscribe linux-kernel\" in\n" "> the body of a message to majordomo@vger.kernel.org\n" "> More majordomo info at http://vger.kernel.org/majordomo-info.html\n" > Please read the FAQ at http://www.tux.org/lkml/ -edef985d00673813f0689b8ca7a8f0efddc13c9cb407d57d85a914ed78050f7b +90035ef2503dc45f7ba4275294236be4dad0f8ec39327a82b5357419fa3bf0e1
diff --git a/a/1.txt b/N2/1.txt index bd0ccbc..bdd8cd2 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,52 +1,42 @@ Quoting Paul Osmialowski (2015-07-26 13:24:08) > 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 = - +> +> 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 Hi Paul, No problem! And thanks for the quick turnaround on your patches so far. -> 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) = - +> 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 =3D "fixed-clock"; -> #clock-cells =3D <0>; -> clock-frequency =3D <50000000>; +> compatible = "fixed-clock"; +> #clock-cells = <0>; +> clock-frequency = <50000000>; > }; -> = - +> > osc1: clock { -> compatible =3D "fixed-clock"; -> #clock-cells =3D <0>; -> clock-frequency =3D <12000000>; +> compatible = "fixed-clock"; +> #clock-cells = <0>; +> clock-frequency = <12000000>; > }; -> = - +> > rtc: clock { -> compatible =3D "fixed-clock"; -> #clock-cells =3D <0>; -> clock-frequency =3D <32768>; +> compatible = "fixed-clock"; +> #clock-cells = <0>; +> clock-frequency = <32768>; > }; -> = - +> > mcgout: clock { -> compatible =3D "fixed-factor-clock"; -> #clock-cells =3D <0>; -> clocks =3D <&osc0>; -> clock-mult =3D <12>; -> clock-div =3D <5>; +> compatible = "fixed-factor-clock"; +> #clock-cells = <0>; +> clocks = <&osc0>; +> clock-mult = <12>; +> clock-div = <5>; > }; I think this is a step backwards. @@ -85,23 +75,21 @@ So why is it listed here as a fixed-factor clock? Is it not a multiplexer? Also, why is it listed here at all? Please take another look at the qcom binding example I linked to in my previous mail. -> = - +> > core: clock { -> compatible =3D "fixed-factor-clock"; -> #clock-cells =3D <0>; -> clocks =3D <&mcgout>; -> clock-mult =3D <1>; -> clock-div =3D <1>; +> compatible = "fixed-factor-clock"; +> #clock-cells = <0>; +> clocks = <&mcgout>; +> clock-mult = <1>; +> clock-div = <1>; > }; -> = - +> > bus: clock { -> compatible =3D "fixed-factor-clock"; -> #clock-cells =3D <0>; -> clocks =3D <&mcgout>; -> clock-mult =3D <1>; -> clock-div =3D <2>; +> compatible = "fixed-factor-clock"; +> #clock-cells = <0>; +> clocks = <&mcgout>; +> clock-mult = <1>; +> clock-div = <2>; These are actually not fixed dividers but programmable dividers. You can probably use drivers/clk/clk-divider.c for these. I'm fine with using @@ -121,54 +109,48 @@ property of some device which *consumes* the clock, not the clock controller or the clock output itself. > }; -> = - +> > soc { -> cmu@0x40047000 { -> compatible =3D "fsl,kinetis-gate-clock"; -> reg =3D <0x40047000 0x1100>; -> = - +> cmu at 0x40047000 { +> compatible = "fsl,kinetis-gate-clock"; +> reg = <0x40047000 0x1100>; +> > mcg_core_gate: clock-gate { -> clocks =3D <&core>; -> #clock-cells =3D <2>; +> clocks = <&core>; +> #clock-cells = <2>; > }; -> = - +> > mcg_bus_gate: clock-gate { -> clocks =3D <&bus>; -> #clock-cells =3D <2>; +> clocks = <&bus>; +> #clock-cells = <2>; > }; -> = - +> > osc0_erclk_gate: clock-gate { -> clocks =3D <&osc0>; -> #clock-cells =3D <2>; +> clocks = <&osc0>; +> #clock-cells = <2>; > }; > }; -> = - -> uart0: serial@4006a000 { -> compatible =3D "fsl,kinetis-lpuart"; -> reg =3D <0x4006a000 0x1000>; -> interrupts =3D <45>, <46>; -> interrupt-names =3D "uart-stat", "uart-err"; -> clocks =3D <&mcg_core_gate 3 10>; +> +> uart0: serial at 4006a000 { +> compatible = "fsl,kinetis-lpuart"; +> reg = <0x4006a000 0x1000>; +> interrupts = <45>, <46>; +> interrupt-names = "uart-stat", "uart-err"; +> clocks = <&mcg_core_gate 3 10>; Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example. -> clock-names =3D "ipg"; -> dmas =3D <&edma 0 2>; -> dma-names =3D "rx"; -> status =3D "disabled"; +> clock-names = "ipg"; +> dmas = <&edma 0 2>; +> dma-names = "rx"; +> status = "disabled"; > }; > }; > }; -> = - +> > As you can see, mcg part is not required anymore. I think the mcg should be required. The mcg is a real IP block on your @@ -179,12 +161,9 @@ that you should. I did a quick grep and didn't find "cmu" anywhere in the reference manual. -> = - -> I guess that the approach above would require split into soc-specific and = - -> board-specific part (as I said, dividers arrangement is something board = - +> +> 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. Splitting is good. Chip-specific stuff can go into the chip-specific @@ -194,165 +173,119 @@ files. The ultimate goal is to make it trivial to add new boards. Regards, Mike -> = - +> > 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 = - +> > > +> > > 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 = - +> > +> > > +> > > 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 ti= -me by = - -> > > reading MCG registers, let me quote commit message from Emcraft git r= -epo: -> > > = - -> > > * Determine in run-time what oscillator module (OSC0 or OSC1) i= -s used +> > > +> > > 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 a= -ll -> > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM = -and +> > +> > > * 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 an= -d = - +> > > +> > > 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 =3D "fixed-clock"; -> > #clock-cells =3D <0>; -> > clock-frequency =3D <50000000>; +> > compatible = "fixed-clock"; +> > #clock-cells = <0>; +> > clock-frequency = <50000000>; > > }; -> > = - +> > > > osc1: clock { -> > compatible =3D "fixed-clock"; -> > #clock-cells =3D <0>; -> > clock-frequency =3D <12000000>; +> > compatible = "fixed-clock"; +> > #clock-cells = <0>; +> > clock-frequency = <12000000>; > > }; -> > = - +> > > > rtc: clock { -> > compatible =3D "fixed-clock"; -> > #clock-cells =3D <0>; -> > clock-frequency =3D <32768>; +> > compatible = "fixed-clock"; +> > #clock-cells = <0>; +> > clock-frequency = <32768>; > > }; -> > = - +> > > > soc: soc { -> > mcg: clock-controller@40064000 { -> > compatible =3D "fsl,kinetis-mcg"; -> > clock-cells =3D <1>; -> > reg =3D <0x40064000 0x14>; -> > clocks =3D <&osc0>, <&osc1>, <&rtc>; -> > clock-names =3D "osc0", "osc1", "rtc"; +> > mcg: clock-controller at 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 =3D "fsl,kinetis-sim"; -> > clock-cells =3D <1>; -> > reg =3D <0x40047000 0x1100>; -> > clocks =3D <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_M= -CGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; -> > clock-names =3D "core", "bus", "flexbus", "flash"; +> > +> > sim: clock-controller at 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 =3D "fsl,kinetis-lpuart"; -> > reg =3D <0x4006a000 0x1000>; -> > clocks =3D <&sim SIM_SCGC4_UART1_CLK>; -> > clock-names =3D "gate"; +> > +> > uart0: serial at 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 @@ -360,98 +293,69 @@ CGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; > > 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/K7= -0P256M150SF3RM.pdf -> > = - +> > +> > [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 ro= -ot = - -> > > 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 = - +> > +> > > +> > > 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 a= -re +> > > >>>> 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 convin= -cing. After -> > > >>> all, they all share the same I/O regs in order to read configurat= -ion. +> > > >>> 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. Th= -at's -> > > >> just a HW design decision and you need to deal with that by protec= -ting -> > > >> the register access, but not by trying to group them artificially = -at +> > > >> 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 own= -s the -> > > > registers and that should be one node in DT, as Paul's first versio= -n does. +> > > > 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-r= -ate -> > > > clocks are controlled through those registers. If they are indeed c= -onfigured -> > > > through the registers, the name is probably wrong and should be cha= -nged +> > > > 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 +> > > linux-arm-kernel at 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 +> the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ diff --git a/a/content_digest b/N2/content_digest index ad41a26..f013fb4 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -5,94 +5,51 @@ "ref\0alpine.LNX.2.00.1507042327490.1296@localhost.localdomain\0" "ref\020150724034229.642.88156@quantum\0" "ref\0alpine.LNX.2.00.1507262211470.28341@localhost.localdomain\0" - "From\0Michael Turquette <mturquette@baylibre.com>\0" - "Subject\0Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC\0" + "From\0mturquette@baylibre.com (Michael Turquette)\0" + "Subject\0[PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC\0" "Date\0Tue, 28 Jul 2015 09:03:47 -0700\0" - "To\0Paul Osmialowski <pawelo@king.net.pl>" - "\0" - "Cc\0Paul Osmialowski <pawelo@king.net.pl>" - Arnd Bergmann <arnd@arndb.de> - Mark Rutland <mark.rutland@arm.com> - Nicolas Pitre <nicolas.pitre@linaro.org> - Linus Walleij <linus.walleij@linaro.org> - Rob Herring <r.herring@freescale.com> - Alexander Potashev <aspotashev@emcraft.com> - Frank Li <Frank.Li@freescale.com> - Jiri Slaby <jslaby@suse.cz> - linux-clk@vger.kernel.org - Russell King <linux@arm.linux.org.uk> - Vinod Koul <vinod.koul@intel.com> - Geert Uytterhoeven <geert@linux-m68k.org> - linux-serial@vger.kernel.org - Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de> - Anson Huang <b20788@freescale.com> - devicetree@vger.kernel.org - Pawel Moll <pawel.moll@arm.com> - Ian Campbell <ijc+devicetree@hellion.org.uk> - Kumar Gala <galak@codeaurora.org> - Yuri Tikhonov <yur@emcraft.com> - linux-gpio@vger.kernel.org - Rob Herring <robh+dt@kernel.org> - Thomas Gleixner <tglx@linutronix.de> - linux-arm-kernel@lists.infradead.org - Sergei Poselenov <sposelenov@emcraft.com> - Paul Bolle <pebolle@tiscali.nl> - Greg Kroah-Hartman <gregkh@linuxfoundation.org> - Stephen Boyd <sboyd@codeaurora.org> - linux-kernel@vger.kernel.org - Jingchang Lu <jingchang.lu@freescale.com> - " dmaengine@vger.kernel.org\0" + "To\0linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "Quoting Paul Osmialowski (2015-07-26 13:24:08)\n" "> Hi Mike,\n" - "> =\n" - "\n" - "> Thank you for spending time on this and pointing me into the right =\n" - "\n" - "> direction. I'm wondering about going even further with it. Assuming that =\n" - "I =\n" - "\n" + "> \n" + "> Thank you for spending time on this and pointing me into the right \n" + "> direction. I'm wondering about going even further with it. Assuming that I \n" "\n" "Hi Paul,\n" "\n" "No problem! And thanks for the quick turnaround on your patches so far.\n" "\n" - "> know everything about my board, I can skip run-time discovery phase (note =\n" - "\n" - "> that the original driver was designed for other Kinetis-based boards too) =\n" - "\n" + "> know everything about my board, I can skip run-time discovery phase (note \n" + "> that the original driver was designed for other Kinetis-based boards too) \n" "> and move everything into DTS, somewhat like this:\n" - "> =\n" - "\n" + "> \n" "> / {\n" "> osc0: clock {\n" - "> compatible =3D \"fixed-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clock-frequency =3D <50000000>;\n" + "> compatible = \"fixed-clock\";\n" + "> #clock-cells = <0>;\n" + "> clock-frequency = <50000000>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> osc1: clock {\n" - "> compatible =3D \"fixed-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clock-frequency =3D <12000000>;\n" + "> compatible = \"fixed-clock\";\n" + "> #clock-cells = <0>;\n" + "> clock-frequency = <12000000>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> rtc: clock {\n" - "> compatible =3D \"fixed-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clock-frequency =3D <32768>;\n" + "> compatible = \"fixed-clock\";\n" + "> #clock-cells = <0>;\n" + "> clock-frequency = <32768>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> mcgout: clock {\n" - "> compatible =3D \"fixed-factor-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clocks =3D <&osc0>;\n" - "> clock-mult =3D <12>;\n" - "> clock-div =3D <5>;\n" + "> compatible = \"fixed-factor-clock\";\n" + "> #clock-cells = <0>;\n" + "> clocks = <&osc0>;\n" + "> clock-mult = <12>;\n" + "> clock-div = <5>;\n" "> };\n" "\n" "I think this is a step backwards.\n" @@ -131,23 +88,21 @@ "multiplexer? Also, why is it listed here at all? Please take another\n" "look at the qcom binding example I linked to in my previous mail.\n" "\n" - "> =\n" - "\n" + "> \n" "> core: clock {\n" - "> compatible =3D \"fixed-factor-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clocks =3D <&mcgout>;\n" - "> clock-mult =3D <1>;\n" - "> clock-div =3D <1>;\n" + "> compatible = \"fixed-factor-clock\";\n" + "> #clock-cells = <0>;\n" + "> clocks = <&mcgout>;\n" + "> clock-mult = <1>;\n" + "> clock-div = <1>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> bus: clock {\n" - "> compatible =3D \"fixed-factor-clock\";\n" - "> #clock-cells =3D <0>;\n" - "> clocks =3D <&mcgout>;\n" - "> clock-mult =3D <1>;\n" - "> clock-div =3D <2>;\n" + "> compatible = \"fixed-factor-clock\";\n" + "> #clock-cells = <0>;\n" + "> clocks = <&mcgout>;\n" + "> clock-mult = <1>;\n" + "> clock-div = <2>;\n" "\n" "These are actually not fixed dividers but programmable dividers. You can\n" "probably use drivers/clk/clk-divider.c for these. I'm fine with using\n" @@ -167,54 +122,48 @@ "controller or the clock output itself.\n" "\n" "> };\n" - "> =\n" - "\n" + "> \n" "> soc {\n" - "> cmu@0x40047000 {\n" - "> compatible =3D \"fsl,kinetis-gate-clock\";\n" - "> reg =3D <0x40047000 0x1100>;\n" - "> =\n" - "\n" + "> cmu at 0x40047000 {\n" + "> compatible = \"fsl,kinetis-gate-clock\";\n" + "> reg = <0x40047000 0x1100>;\n" + "> \n" "> mcg_core_gate: clock-gate {\n" - "> clocks =3D <&core>;\n" - "> #clock-cells =3D <2>;\n" + "> clocks = <&core>;\n" + "> #clock-cells = <2>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> mcg_bus_gate: clock-gate {\n" - "> clocks =3D <&bus>;\n" - "> #clock-cells =3D <2>;\n" + "> clocks = <&bus>;\n" + "> #clock-cells = <2>;\n" "> };\n" - "> =\n" - "\n" + "> \n" "> osc0_erclk_gate: clock-gate {\n" - "> clocks =3D <&osc0>;\n" - "> #clock-cells =3D <2>;\n" + "> clocks = <&osc0>;\n" + "> #clock-cells = <2>;\n" "> };\n" "> };\n" - "> =\n" - "\n" - "> uart0: serial@4006a000 {\n" - "> compatible =3D \"fsl,kinetis-lpuart\";\n" - "> reg =3D <0x4006a000 0x1000>;\n" - "> interrupts =3D <45>, <46>;\n" - "> interrupt-names =3D \"uart-stat\", \"uart-err\";\n" - "> clocks =3D <&mcg_core_gate 3 10>;\n" + "> \n" + "> uart0: serial at 4006a000 {\n" + "> compatible = \"fsl,kinetis-lpuart\";\n" + "> reg = <0x4006a000 0x1000>;\n" + "> interrupts = <45>, <46>;\n" + "> interrupt-names = \"uart-stat\", \"uart-err\";\n" + "> clocks = <&mcg_core_gate 3 10>;\n" "\n" "Magic numbers are not good. dtc has been able to use preprocessor macros\n" "for a while now which means we can use constants instead of magic\n" "numbers. Please look at the shared header in the qcom binding for an\n" "example.\n" "\n" - "> clock-names =3D \"ipg\";\n" - "> dmas =3D <&edma 0 2>;\n" - "> dma-names =3D \"rx\";\n" - "> status =3D \"disabled\";\n" + "> clock-names = \"ipg\";\n" + "> dmas = <&edma 0 2>;\n" + "> dma-names = \"rx\";\n" + "> status = \"disabled\";\n" "> };\n" "> };\n" "> };\n" - "> =\n" - "\n" + "> \n" "> As you can see, mcg part is not required anymore.\n" "\n" "I think the mcg should be required. The mcg is a real IP block on your\n" @@ -225,12 +174,9 @@ "I did a quick grep and didn't find \"cmu\" anywhere in the reference\n" "manual.\n" "\n" - "> =\n" - "\n" - "> I guess that the approach above would require split into soc-specific and =\n" - "\n" - "> board-specific part (as I said, dividers arrangement is something board =\n" - "\n" + "> \n" + "> I guess that the approach above would require split into soc-specific and \n" + "> board-specific part (as I said, dividers arrangement is something board \n" "> specific), but I wonder what you thing about this proposal.\n" "\n" "Splitting is good. Chip-specific stuff can go into the chip-specific\n" @@ -240,165 +186,119 @@ "Regards,\n" "Mike\n" "\n" - "> =\n" - "\n" + "> \n" "> Thanks,\n" "> Paul\n" - "> =\n" - "\n" + "> \n" "> On Thu, 23 Jul 2015, Michael Turquette wrote:\n" - "> =\n" - "\n" + "> \n" "> > Quoting Paul Osmialowski (2015-07-04 14:50:03)\n" "> > > Hi Arnd,\n" - "> > > =\n" - "\n" - "> > > I'm attaching excerpt from Kinetis reference manual that may make =\n" - "\n" + "> > > \n" + "> > > I'm attaching excerpt from Kinetis reference manual that may make \n" "> > > situation clearer.\n" - "> > =\n" - "\n" + "> > \n" "> > Hi Paul,\n" - "> > =\n" - "\n" + "> > \n" "> > Can you please post the patch in the body of the email instead of an\n" "> > attachment? It makes it easier to review. Another small nitpick is that\n" "> > the $SUBJECT for this patch might be better off as something like:\n" - "> > =\n" - "\n" + "> > \n" "> > clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC\n" - "> > =\n" - "\n" + "> > \n" "> > At least it helps me find the patch I care about when skimming the\n" "> > series ;-)\n" - "> > =\n" - "\n" - "> > > =\n" - "\n" - "> > > These MCG and SIM registers are used only to determine configuration =\n" - "\n" + "> > \n" + "> > > \n" + "> > > These MCG and SIM registers are used only to determine configuration \n" "> > > (clock fixed rates and clock signal origins) at run time.\n" - "> > > =\n" - "\n" - "> > > Namely, the real MCGOUTCLK source (in the middle) which is the parent=\n" - " for =\n" - "\n" - "> > > core clock (CCLK) and peripheral clock (PCLK) is determined at run ti=\n" - "me by =\n" - "\n" - "> > > reading MCG registers, let me quote commit message from Emcraft git r=\n" - "epo:\n" - "> > > =\n" - "\n" - "> > > * Determine in run-time what oscillator module (OSC0 or OSC1) i=\n" - "s used\n" + "> > > \n" + "> > > Namely, the real MCGOUTCLK source (in the middle) which is the parent for \n" + "> > > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by \n" + "> > > reading MCG registers, let me quote commit message from Emcraft git repo:\n" + "> > > \n" + "> > > * Determine in run-time what oscillator module (OSC0 or OSC1) is used\n" "> > > as clock source for the main PLL.\n" - "> > =\n" - "\n" + "> > \n" "> > According to [0] there are three options: a 32k RTC osc clock and osc0\n" "> > both feed into a mux. You should model this 32k clock with the\n" "> > fixed-rate binding.\n" - "> > =\n" - "\n" - "> > > * When OSC1 is selected, assume its frequency to be 12 MHz on a=\n" - "ll\n" - "> > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM =\n" - "and\n" + "> > \n" + "> > > * When OSC1 is selected, assume its frequency to be 12 MHz on all\n" + "> > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and\n" "> > > TWR-K70F120M boards).\n" - "> > > =\n" - "\n" - "> > > In my .dts I'm trying to possibly follow real clock hierarchy, but to=\n" - " go =\n" - "\n" - "> > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. =\n" - "by =\n" - "\n" - "> > > U-boot. But that's too demanding for any potential users of this BSP.=\n" - " So =\n" - "\n" - "> > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK an=\n" - "d =\n" - "\n" + "> > > \n" + "> > > In my .dts I'm trying to possibly follow real clock hierarchy, but to go \n" + "> > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by \n" + "> > > U-boot. But that's too demanding for any potential users of this BSP. So \n" + "> > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and \n" "> > > PCLK.\n" - "> > =\n" - "\n" + "> > \n" "> > I'm confused. The point of device tree is to solve problems like this;\n" "> > i.e. board-specific differences such as different oscillator\n" "> > frequencies.\n" - "> > =\n" - "\n" + "> > \n" "> > OSC0 and OSC1 should each be a fixed-rate clock in your board-specific\n" "> > TWR-K70F120M DTS (not a chip-specific file). They do not belong in the\n" "> > cmu node, and they should use the \"fixed-clock\" binding. The 32k RTC osc\n" "> > can probably go in your chip-specific .dtsi as a fixed-rate clock since\n" "> > it appears to mandated in the reference manual[0].\n" - "> > =\n" - "\n" + "> > \n" "> > These three fixed-rate clocks are your root clock nodes. Customers only\n" "> > need to worry about this if they spin a board, and then they will need\n" "> > to populate the frequencies of OSC0 and OSC1 in their board-specific\n" "> > .dts.\n" - "> > =\n" - "\n" + "> > \n" "> > Please break clk-kinetis.c into two files:\n" "> > drivers/clk/kinetis/clk-mcg.c\n" "> > drivers/clk/kinetis/clk-sim.c\n" - "> > =\n" - "\n" + "> > \n" "> > Below is what your binding/dts should look like:\n" - "> > =\n" - "\n" + "> > \n" "> > {\n" "> > osc0: clock {\n" - "> > compatible =3D \"fixed-clock\";\n" - "> > #clock-cells =3D <0>;\n" - "> > clock-frequency =3D <50000000>;\n" + "> > compatible = \"fixed-clock\";\n" + "> > #clock-cells = <0>;\n" + "> > clock-frequency = <50000000>;\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > osc1: clock {\n" - "> > compatible =3D \"fixed-clock\";\n" - "> > #clock-cells =3D <0>;\n" - "> > clock-frequency =3D <12000000>;\n" + "> > compatible = \"fixed-clock\";\n" + "> > #clock-cells = <0>;\n" + "> > clock-frequency = <12000000>;\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > rtc: clock {\n" - "> > compatible =3D \"fixed-clock\";\n" - "> > #clock-cells =3D <0>;\n" - "> > clock-frequency =3D <32768>;\n" + "> > compatible = \"fixed-clock\";\n" + "> > #clock-cells = <0>;\n" + "> > clock-frequency = <32768>;\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > soc: soc {\n" - "> > mcg: clock-controller@40064000 {\n" - "> > compatible =3D \"fsl,kinetis-mcg\";\n" - "> > clock-cells =3D <1>;\n" - "> > reg =3D <0x40064000 0x14>;\n" - "> > clocks =3D <&osc0>, <&osc1>, <&rtc>;\n" - "> > clock-names =3D \"osc0\", \"osc1\", \"rtc\";\n" + "> > mcg: clock-controller at 40064000 {\n" + "> > compatible = \"fsl,kinetis-mcg\";\n" + "> > clock-cells = <1>;\n" + "> > reg = <0x40064000 0x14>;\n" + "> > clocks = <&osc0>, <&osc1>, <&rtc>;\n" + "> > clock-names = \"osc0\", \"osc1\", \"rtc\";\n" "> > };\n" - "> > =\n" - "\n" - "> > sim: clock-controller@40047000 {\n" - "> > compatible =3D \"fsl,kinetis-sim\";\n" - "> > clock-cells =3D <1>;\n" - "> > reg =3D <0x40047000 0x1100>;\n" - "> > clocks =3D <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_M=\n" - "CGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;\n" - "> > clock-names =3D \"core\", \"bus\", \"flexbus\", \"flash\";\n" + "> > \n" + "> > sim: clock-controller at 40047000 {\n" + "> > compatible = \"fsl,kinetis-sim\";\n" + "> > clock-cells = <1>;\n" + "> > reg = <0x40047000 0x1100>;\n" + "> > clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;\n" + "> > clock-names = \"core\", \"bus\", \"flexbus\", \"flash\";\n" "> > };\n" "> > };\n" - "> > =\n" - "\n" - "> > uart0: serial@4006a000 {\n" - "> > compatible =3D \"fsl,kinetis-lpuart\";\n" - "> > reg =3D <0x4006a000 0x1000>;\n" - "> > clocks =3D <&sim SIM_SCGC4_UART1_CLK>;\n" - "> > clock-names =3D \"gate\";\n" + "> > \n" + "> > uart0: serial at 4006a000 {\n" + "> > compatible = \"fsl,kinetis-lpuart\";\n" + "> > reg = <0x4006a000 0x1000>;\n" + "> > clocks = <&sim SIM_SCGC4_UART1_CLK>;\n" + "> > clock-names = \"gate\";\n" "> > };\n" - "> > =\n" - "\n" + "> > \n" "> > I removed the interrupts and dma stuff from the uart0 node for clarity.\n" "> > The above is the only style of binding that I have been accepting for\n" "> > some time; first declare the clock controller and establish its register\n" @@ -406,100 +306,71 @@ "> > the controller plus an offset corresponding to a unique clock. The\n" "> > clock-names property makes it really easy to use with the clkdev stuff\n" "> > (e.g. clk_get()).\n" - "> > =\n" - "\n" + "> > \n" "> > I've covered this before on the mailing list so here is a link\n" "> > describing how the qcom bindings do it in detail:\n" - "> > =\n" - "\n" + "> > \n" "> > http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum>\n" - "> > =\n" - "\n" + "> > \n" "> > Technically you could encode the same bits as sub-nodes of the mcg and\n" "> > sim nodes, but the shared header is how the magic happens with the\n" "> > driver so it's best to keep the clock controller binding small and\n" "> > light.\n" - "> > =\n" - "\n" + "> > \n" "> > I think this means you can also get rid of kinetis_of_clk_get_name and\n" "> > kinetis_clk_gate_get but my brain is tired so I'll leave that as an\n" "> > exercise to the reader.\n" - "> > =\n" - "\n" - "> > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K7=\n" - "0P256M150SF3RM.pdf\n" - "> > =\n" - "\n" + "> > \n" + "> > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf\n" + "> > \n" "> > Regards,\n" "> > Mike\n" - "> > =\n" - "\n" - "> > > =\n" - "\n" - "> > > In my most recent version I added OSC0ERCLK explicitly as one more ro=\n" - "ot =\n" - "\n" - "> > > clock, since it is also used directly (through CG reg. 1 bit 0) by =\n" - "\n" - "> > > Freescale fec network device whose in-tree driver I'm trying to make =\n" - "\n" + "> > \n" + "> > > \n" + "> > > In my most recent version I added OSC0ERCLK explicitly as one more root \n" + "> > > clock, since it is also used directly (through CG reg. 1 bit 0) by \n" + "> > > Freescale fec network device whose in-tree driver I'm trying to make \n" "> > > usable for Kinetis.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > On Sat, 4 Jul 2015, Arnd Bergmann wrote:\n" - "> > > =\n" - "\n" + "> > > \n" "> > > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:\n" "> > > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote:\n" "> > > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote:\n" "> > > >>>\n" - "> > > >>>> I wonder if you could move out the fixed rate clocks into their =\n" - "own\n" - "> > > >>>> nodes. Are they actually controlled by the same block? If they a=\n" - "re\n" + "> > > >>>> I wonder if you could move out the fixed rate clocks into their own\n" + "> > > >>>> nodes. Are they actually controlled by the same block? If they are\n" "> > > >>>> just fixed, you can use the normal binding for fixed rate clocks\n" "> > > >>>> and only describe the clocks that are related to the driver.\n" "> > > >>>\n" - "> > > >>> In my view having these clocks grouped together looks more convin=\n" - "cing. After\n" - "> > > >>> all, they all share the same I/O regs in order to read configurat=\n" - "ion.\n" + "> > > >>> In my view having these clocks grouped together looks more convincing. After\n" + "> > > >>> all, they all share the same I/O regs in order to read configuration.\n" "> > > >>\n" - "> > > >> The fact that they share a register is not making them a group. Th=\n" - "at's\n" - "> > > >> just a HW design decision and you need to deal with that by protec=\n" - "ting\n" - "> > > >> the register access, but not by trying to group them artificially =\n" - "at\n" + "> > > >> The fact that they share a register is not making them a group. That's\n" + "> > > >> just a HW design decision and you need to deal with that by protecting\n" + "> > > >> the register access, but not by trying to group them artificially at\n" "> > > >> the functional level.\n" "> > > >\n" - "> > > > I'd disagree with that: The clock controller is the device that own=\n" - "s the\n" - "> > > > registers and that should be one node in DT, as Paul's first versio=\n" - "n does.\n" + "> > > > I'd disagree with that: The clock controller is the device that owns the\n" + "> > > > registers and that should be one node in DT, as Paul's first version does.\n" "> > > >\n" - "> > > > The part I'm still struggling with is understanding how the fixed-r=\n" - "ate\n" - "> > > > clocks are controlled through those registers. If they are indeed c=\n" - "onfigured\n" - "> > > > through the registers, the name is probably wrong and should be cha=\n" - "nged\n" + "> > > > The part I'm still struggling with is understanding how the fixed-rate\n" + "> > > > clocks are controlled through those registers. If they are indeed configured\n" + "> > > > through the registers, the name is probably wrong and should be changed\n" "> > > > to whatever kind of non-fixed clock this is.\n" "> > > >\n" "> > > > Arnd\n" "> > > >\n" - "> > > =\n" - "\n" + "> > > \n" "> > > _______________________________________________\n" "> > > linux-arm-kernel mailing list\n" - "> > > linux-arm-kernel@lists.infradead.org\n" + "> > > linux-arm-kernel at lists.infradead.org\n" "> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel\n" - "> > =\n" - "\n" + "> > \n" "> --\n" "> To unsubscribe from this list: send the line \"unsubscribe linux-kernel\" in\n" - "> the body of a message to majordomo@vger.kernel.org\n" + "> the body of a message to majordomo at vger.kernel.org\n" "> More majordomo info at http://vger.kernel.org/majordomo-info.html\n" > Please read the FAQ at http://www.tux.org/lkml/ -edef985d00673813f0689b8ca7a8f0efddc13c9cb407d57d85a914ed78050f7b +c7332144520c2c561ba9ca93636000622d60ccbe78ea608a683166b336de0183
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.