diff for duplicates of <20150729230517.642.66061@quantum> diff --git a/a/1.txt b/N1/1.txt index 4b47760..9dcb5d1 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,79 +1,52 @@ Quoting Paul Osmialowski (2015-07-28 13:30:17) > Hi Mike, -> = - -> My trouble is that now I'm dealing with two conradictory opinions on how = - -> this driver should be written. The one you presented in your previous pos= -t = - -> assumes that there will be a header file with defines shared between the = - -> clock driver and DTS, also with clock gating details hidden behind some = - +> +> My trouble is that now I'm dealing with two conradictory opinions on how +> this driver should be written. The one you presented in your previous post +> assumes that there will be a header file with defines shared between the +> clock driver and DTS, also with clock gating details hidden behind some > additional level of indirection, e.g.: -> = - -> clocks =3D <&sim SIM_SCGC4_UART1_CLK>; -> = - -> Note that I've been through this at the very beginning, though the names = - +> +> clocks = <&sim SIM_SCGC4_UART1_CLK>; +> +> Note that I've been through this at the very beginning, though the names > I used have been bit different, e.g.: -> = - +> > #define KINETIS_CG_UART1 KINETIS_MKCG(3, 11) /* SIM_SCGC4[11] */ -> = - +> > This was rejected with a comment by Arnd: -> = - +> > Instead of using a triple indirection here, just put the tuples -> in the DT directly using #clock-cells=3D<2>, and get rid of both this +> in the DT directly using #clock-cells=<2>, and get rid of both this > header file and the dt-bindings/clock/kinetis-mcg.h file. Arnd, are you OK with my type of binding description now that I explained it with some examples? -> = - -> So I dropped all of these includes and started to use magic numbers (as = - -> you put it). Now I need to go one way or another or even go the third way= -: = - -> extend #clock-cells to <3> and address it like: <&sim parent_clock_id = - +> +> So I dropped all of these includes and started to use magic numbers (as +> you put it). Now I need to go one way or another or even go the third way: +> extend #clock-cells to <3> and address it like: <&sim parent_clock_id > scgc_register_number bit_index_number>. Paul, -From=20my understanding the DT folks do not like register-level or +>From my understanding the DT folks do not like register-level or bit-level details going into DT. It is better to handle the clock signals as abstract resources and link a provider and consumer with a simple phandle plus an index representing that abstract resource (i.e. the clock output signal). -> = - -> Reading your previous post I'm starting to feel that it would bring me = - -> closer to final acceptance if I stick to what you proposed in that post = - -> (I'm really grateful to you for writting so huge chunk of DTS for me!), s= -o = - +> +> Reading your previous post I'm starting to feel that it would bring me +> closer to final acceptance if I stick to what you proposed in that post +> (I'm really grateful to you for writting so huge chunk of DTS for me!), so > I'll probably adopt that. -> = - -> You're right about my "get things up and running" attitude - currently I = - -> want to develop things extensively (cover as much subsystems as = - -> possible) and then at some stage switch to intensive approach. This board = - -> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot = - +> +> You're right about my "get things up and running" attitude - currently I +> want to develop things extensively (cover as much subsystems as +> possible) and then at some stage switch to intensive approach. This board +> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot > of coding opportunity in this field in the future. I'm happy to take clock drivers and add my Reviewed-by to .dts files @@ -90,79 +63,58 @@ later if it needs to. Regards, Mike -> = - +> > Thanks, > Paul -> = - +> > On Tue, 28 Jul 2015, Michael Turquette wrote: -> = - +> > > 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 t= -hat 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. -> > = - +> > > > Did you look at the qcom clock binding and read the email where I > > detailed how that binding works? -> > = - +> > > > The point of that type of binding is to not shove per-clock data into > > DT, but instead to declare every clock controller IP block (e.g. the > > device) as well as every board-level clock (e.g. as osc that feeds into @@ -170,444 +122,315 @@ too) = > > create linkage between the clock providers and the clock consumers by > > using phandles + an index. Linux device drivers tap into this by using > > clk_get() and using the "clock-names" property from DT. -> > = - +> > > > Put another way: we mostly use DT to model "devices". That is open to > > interpretation for but for clock-related stuff we typically interpret > > the clock controller as the device, not the individual clock outputs > > coming out of the controller. -> > = - +> > > > Note that a clock controller IP block may be both a provider and a > > consumer. I/O controllers are a very common type of consumer (e.g. USB > > host controller, MMC controller, GPU, etc). -> > = - +> > > > Additionally, from my reading of the reference manual, mcgout is defined > > as: -> > = - +> > > > """ > > MCG output of either IRC, MCGFLLCLK MCGPLL0CLK, > > MCGPLL1CLK, or MCG's external reference clock that > > sources the core, system, bus, FlexBus, and flash clock. It is > > also an option for the debug trace clock. > > """ -> > = - +> > > > 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 > > fixed-dividers for the initial merge just to get things up and running > > if that is your strategy, but you'll need to revisit them later on when > > you need more flexible support for other boards. -> > = - +> > > > Again, I'm not sure why these clocks are enumerated in DT. Why not just > > enumerate your mcg clock controller and your sim clock controller? If > > you want to be a perfectionist then it appears that there is an osc > > clock controller upstream from the mcg controller as well ;-) -> > = - +> > > > It occurs to me that maybe you are trying to use fixed-factor clocks so > > that you can program a sane default rate? We use the > > assigned-clock-rates property for that. Note that this value is a > > 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 > > SoC, according to my reading of your technical reference manual. Just > > because you can model a few of its output clocks in dts does not mean > > 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 boa= -rd = - +> > +> > > +> > > 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 > > dtsi file. The board-level (osc) stuff can go into the individual board > > 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 +> > > > 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 configurat= -ion = - +> > > > +> > > > > +> > > > > 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 pa= -rent for = - -> > > > > core clock (CCLK) and peripheral clock (PCLK) is determined at ru= -n time by = - -> > > > > reading MCG registers, let me quote commit message from Emcraft g= -it repo: -> > > > > = - -> > > > > * Determine in run-time what oscillator module (OSC0 or OSC= -1) is 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 o= -sc0 +> > > > +> > > > 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 +> > > > +> > > > > * 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, bu= -t 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 CCL= -K and = - +> > > > > +> > > > > 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 th= -is; +> > > > +> > > > 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-speci= -fic -> > > > 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 RT= -C osc -> > > > can probably go in your chip-specific .dtsi as a fixed-rate clock s= -ince +> > > > +> > > > 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 n= -eed +> > > > +> > > > 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 M= -CG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; -> > > > clock-names =3D "core", "bus", "flexbus", "fl= -ash"; +> > > > 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 clar= -ity. -> > > > The above is the only style of binding that I have been accepting f= -or -> > > > some time; first declare the clock controller and establish its reg= -ister -> > > > space, and then consumers can consume clocks by providing the phand= -le to +> > > > +> > > > 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 st= -uff +> > > > 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 +> > > > +> > > > 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 +> > > > +> > > > 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_manua= -l/K70P256M150SF3RM.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 mor= -e root = - -> > > > > clock, since it is also used directly (through CG reg. 1 bit 0) b= -y = - -> > > > > Freescale fec network device whose in-tree driver I'm trying to m= -ake = - +> > > > +> > > > > +> > > > > 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 th= -eir own -> > > > > >>>> nodes. Are they actually controlled by the same block? If th= -ey are -> > > > > >>>> just fixed, you can use the normal binding for fixed rate cl= -ocks +> > > > > >>>> 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 co= -nvincing. After -> > > > > >>> all, they all share the same I/O regs in order to read config= -uration. +> > > > > >>> 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 pr= -otecting -> > > > > >> the register access, but not by trying to group them artificia= -lly 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= - owns the -> > > > > > registers and that should be one node in DT, as Paul's first ve= -rsion 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 fix= -ed-rate -> > > > > > clocks are controlled through those registers. If they are inde= -ed configured -> > > > > > through the registers, the name is probably wrong and should be= - changed +> > > > > > 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-kerne= -l" in +> > > 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/ -> > = - +> > > -- > 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 6121c06..960c21e 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -10,11 +10,7 @@ "From\0Michael Turquette <mturquette@baylibre.com>\0" "Subject\0Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC\0" "Date\0Wed, 29 Jul 2015 16:05:17 -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> @@ -23,105 +19,72 @@ 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-28 13:30:17)\n" "> Hi Mike,\n" - "> =\n" - "\n" - "> My trouble is that now I'm dealing with two conradictory opinions on how =\n" - "\n" - "> this driver should be written. The one you presented in your previous pos=\n" - "t =\n" - "\n" - "> assumes that there will be a header file with defines shared between the =\n" - "\n" - "> clock driver and DTS, also with clock gating details hidden behind some =\n" - "\n" + "> \n" + "> My trouble is that now I'm dealing with two conradictory opinions on how \n" + "> this driver should be written. The one you presented in your previous post \n" + "> assumes that there will be a header file with defines shared between the \n" + "> clock driver and DTS, also with clock gating details hidden behind some \n" "> additional level of indirection, e.g.:\n" - "> =\n" - "\n" - "> clocks =3D <&sim SIM_SCGC4_UART1_CLK>;\n" - "> =\n" - "\n" - "> Note that I've been through this at the very beginning, though the names =\n" - "\n" + "> \n" + "> clocks = <&sim SIM_SCGC4_UART1_CLK>;\n" + "> \n" + "> Note that I've been through this at the very beginning, though the names \n" "> I used have been bit different, e.g.:\n" - "> =\n" - "\n" + "> \n" "> #define KINETIS_CG_UART1 KINETIS_MKCG(3, 11) /* SIM_SCGC4[11] */\n" - "> =\n" - "\n" + "> \n" "> This was rejected with a comment by Arnd:\n" - "> =\n" - "\n" + "> \n" "> Instead of using a triple indirection here, just put the tuples\n" - "> in the DT directly using #clock-cells=3D<2>, and get rid of both this\n" + "> in the DT directly using #clock-cells=<2>, and get rid of both this\n" "> header file and the dt-bindings/clock/kinetis-mcg.h file.\n" "\n" "Arnd, are you OK with my type of binding description now that I\n" "explained it with some examples?\n" "\n" - "> =\n" - "\n" - "> So I dropped all of these includes and started to use magic numbers (as =\n" - "\n" - "> you put it). Now I need to go one way or another or even go the third way=\n" - ": =\n" - "\n" - "> extend #clock-cells to <3> and address it like: <&sim parent_clock_id =\n" - "\n" + "> \n" + "> So I dropped all of these includes and started to use magic numbers (as \n" + "> you put it). Now I need to go one way or another or even go the third way: \n" + "> extend #clock-cells to <3> and address it like: <&sim parent_clock_id \n" "> scgc_register_number bit_index_number>.\n" "\n" "Paul,\n" "\n" - "From=20my understanding the DT folks do not like register-level or\n" + ">From my understanding the DT folks do not like register-level or\n" "bit-level details going into DT. It is better to handle the clock\n" "signals as abstract resources and link a provider and consumer with a\n" "simple phandle plus an index representing that abstract resource (i.e.\n" "the clock output signal).\n" "\n" - "> =\n" - "\n" - "> Reading your previous post I'm starting to feel that it would bring me =\n" - "\n" - "> closer to final acceptance if I stick to what you proposed in that post =\n" - "\n" - "> (I'm really grateful to you for writting so huge chunk of DTS for me!), s=\n" - "o =\n" - "\n" + "> \n" + "> Reading your previous post I'm starting to feel that it would bring me \n" + "> closer to final acceptance if I stick to what you proposed in that post \n" + "> (I'm really grateful to you for writting so huge chunk of DTS for me!), so \n" "> I'll probably adopt that.\n" - "> =\n" - "\n" - "> You're right about my \"get things up and running\" attitude - currently I =\n" - "\n" - "> want to develop things extensively (cover as much subsystems as =\n" - "\n" - "> possible) and then at some stage switch to intensive approach. This board =\n" - "\n" - "> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot =\n" - "\n" + "> \n" + "> You're right about my \"get things up and running\" attitude - currently I \n" + "> want to develop things extensively (cover as much subsystems as \n" + "> possible) and then at some stage switch to intensive approach. This board \n" + "> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot \n" "> of coding opportunity in this field in the future.\n" "\n" "I'm happy to take clock drivers and add my Reviewed-by to .dts files\n" @@ -138,79 +101,58 @@ "Regards,\n" "Mike\n" "\n" - "> =\n" - "\n" + "> \n" "> Thanks,\n" "> Paul\n" - "> =\n" - "\n" + "> \n" "> On Tue, 28 Jul 2015, Michael Turquette wrote:\n" - "> =\n" - "\n" + "> \n" "> > 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 t=\n" - "hat I =\n" - "\n" - "> > =\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" - "\n" + "> > \n" "> > No problem! And thanks for the quick turnaround on your patches so far.\n" - "> > =\n" - "\n" - "> > > know everything about my board, I can skip run-time discovery phase (=\n" - "note =\n" - "\n" - "> > > that the original driver was designed for other Kinetis-based boards =\n" - "too) =\n" - "\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" - "\n" + "> > \n" "> > I think this is a step backwards.\n" - "> > =\n" - "\n" + "> > \n" "> > Did you look at the qcom clock binding and read the email where I\n" "> > detailed how that binding works?\n" - "> > =\n" - "\n" + "> > \n" "> > The point of that type of binding is to not shove per-clock data into\n" "> > DT, but instead to declare every clock controller IP block (e.g. the\n" "> > device) as well as every board-level clock (e.g. as osc that feeds into\n" @@ -218,448 +160,319 @@ "> > create linkage between the clock providers and the clock consumers by\n" "> > using phandles + an index. Linux device drivers tap into this by using\n" "> > clk_get() and using the \"clock-names\" property from DT.\n" - "> > =\n" - "\n" + "> > \n" "> > Put another way: we mostly use DT to model \"devices\". That is open to\n" "> > interpretation for but for clock-related stuff we typically interpret\n" "> > the clock controller as the device, not the individual clock outputs\n" "> > coming out of the controller.\n" - "> > =\n" - "\n" + "> > \n" "> > Note that a clock controller IP block may be both a provider and a\n" "> > consumer. I/O controllers are a very common type of consumer (e.g. USB\n" "> > host controller, MMC controller, GPU, etc).\n" - "> > =\n" - "\n" + "> > \n" "> > Additionally, from my reading of the reference manual, mcgout is defined\n" "> > as:\n" - "> > =\n" - "\n" + "> > \n" "> > \"\"\"\n" "> > MCG output of either IRC, MCGFLLCLK MCGPLL0CLK,\n" "> > MCGPLL1CLK, or MCG's external reference clock that\n" "> > sources the core, system, bus, FlexBus, and flash clock. It is\n" "> > also an option for the debug trace clock.\n" "> > \"\"\"\n" - "> > =\n" - "\n" + "> > \n" "> > So why is it listed here as a fixed-factor clock? Is it not a\n" "> > 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" + "> > \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" - "> > =\n" - "\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" "> > fixed-dividers for the initial merge just to get things up and running\n" "> > if that is your strategy, but you'll need to revisit them later on when\n" "> > you need more flexible support for other boards.\n" - "> > =\n" - "\n" + "> > \n" "> > Again, I'm not sure why these clocks are enumerated in DT. Why not just\n" "> > enumerate your mcg clock controller and your sim clock controller? If\n" "> > you want to be a perfectionist then it appears that there is an osc\n" "> > clock controller upstream from the mcg controller as well ;-)\n" - "> > =\n" - "\n" + "> > \n" "> > It occurs to me that maybe you are trying to use fixed-factor clocks so\n" "> > that you can program a sane default rate? We use the\n" "> > assigned-clock-rates property for that. Note that this value is a\n" "> > property of some device which *consumes* the clock, not the clock\n" "> > controller or the clock output itself.\n" - "> > =\n" - "\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" - "> > =\n" - "\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" - "\n" - "> > > clock-names =3D \"ipg\";\n" - "> > > dmas =3D <&edma 0 2>;\n" - "> > > dma-names =3D \"rx\";\n" - "> > > status =3D \"disabled\";\n" + "> > \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" - "\n" + "> > \n" "> > I think the mcg should be required. The mcg is a real IP block on your\n" "> > SoC, according to my reading of your technical reference manual. Just\n" "> > because you can model a few of its output clocks in dts does not mean\n" "> > that you should.\n" - "> > =\n" - "\n" + "> > \n" "> > I did a quick grep and didn't find \"cmu\" anywhere in the reference\n" "> > manual.\n" - "> > =\n" - "\n" - "> > > =\n" - "\n" - "> > > I guess that the approach above would require split into soc-specific=\n" - " and =\n" - "\n" - "> > > board-specific part (as I said, dividers arrangement is something boa=\n" - "rd =\n" - "\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" - "\n" + "> > \n" "> > Splitting is good. Chip-specific stuff can go into the chip-specific\n" "> > dtsi file. The board-level (osc) stuff can go into the individual board\n" "> > files. The ultimate goal is to make it trivial to add new boards.\n" - "> > =\n" - "\n" + "> > \n" "> > Regards,\n" "> > Mike\n" - "> > =\n" - "\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 =\n" - "that\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 configurat=\n" - "ion =\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 pa=\n" - "rent for =\n" - "\n" - "> > > > > core clock (CCLK) and peripheral clock (PCLK) is determined at ru=\n" - "n time by =\n" - "\n" - "> > > > > reading MCG registers, let me quote commit message from Emcraft g=\n" - "it repo:\n" - "> > > > > =\n" - "\n" - "> > > > > * Determine in run-time what oscillator module (OSC0 or OSC=\n" - "1) is 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" - "> > > > According to [0] there are three options: a 32k RTC osc clock and o=\n" - "sc0\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 =\n" - "on all\n" - "> > > > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-=\n" - "SOM 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, bu=\n" - "t to go =\n" - "\n" - "> > > > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e=\n" - ".g. by =\n" - "\n" - "> > > > > U-boot. But that's too demanding for any potential users of this =\n" - "BSP. So =\n" - "\n" - "> > > > > let's asume that MCGOUTCLK is the root clock and a parent for CCL=\n" - "K and =\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" - "> > > > I'm confused. The point of device tree is to solve problems like th=\n" - "is;\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" - "> > > > OSC0 and OSC1 should each be a fixed-rate clock in your board-speci=\n" - "fic\n" - "> > > > TWR-K70F120M DTS (not a chip-specific file). They do not belong in =\n" - "the\n" - "> > > > cmu node, and they should use the \"fixed-clock\" binding. The 32k RT=\n" - "C osc\n" - "> > > > can probably go in your chip-specific .dtsi as a fixed-rate clock s=\n" - "ince\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" - "> > > > These three fixed-rate clocks are your root clock nodes. Customers =\n" - "only\n" - "> > > > need to worry about this if they spin a board, and then they will n=\n" - "eed\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 M=\n" - "CG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;\n" - "> > > > clock-names =3D \"core\", \"bus\", \"flexbus\", \"fl=\n" - "ash\";\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" - "> > > > I removed the interrupts and dma stuff from the uart0 node for clar=\n" - "ity.\n" - "> > > > The above is the only style of binding that I have been accepting f=\n" - "or\n" - "> > > > some time; first declare the clock controller and establish its reg=\n" - "ister\n" - "> > > > space, and then consumers can consume clocks by providing the phand=\n" - "le to\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" + "> > > > space, and then consumers can consume clocks by providing the phandle to\n" "> > > > the controller plus an offset corresponding to a unique clock. The\n" - "> > > > clock-names property makes it really easy to use with the clkdev st=\n" - "uff\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" - "> > > > Technically you could encode the same bits as sub-nodes of the mcg =\n" - "and\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" - "> > > > I think this means you can also get rid of kinetis_of_clk_get_name =\n" - "and\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_manua=\n" - "l/K70P256M150SF3RM.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 mor=\n" - "e root =\n" - "\n" - "> > > > > clock, since it is also used directly (through CG reg. 1 bit 0) b=\n" - "y =\n" - "\n" - "> > > > > Freescale fec network device whose in-tree driver I'm trying to m=\n" - "ake =\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 th=\n" - "eir own\n" - "> > > > > >>>> nodes. Are they actually controlled by the same block? If th=\n" - "ey are\n" - "> > > > > >>>> just fixed, you can use the normal binding for fixed rate cl=\n" - "ocks\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 co=\n" - "nvincing. After\n" - "> > > > > >>> all, they all share the same I/O regs in order to read config=\n" - "uration.\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=\n" - ". That's\n" - "> > > > > >> just a HW design decision and you need to deal with that by pr=\n" - "otecting\n" - "> > > > > >> the register access, but not by trying to group them artificia=\n" - "lly 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=\n" - " owns the\n" - "> > > > > > registers and that should be one node in DT, as Paul's first ve=\n" - "rsion 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 fix=\n" - "ed-rate\n" - "> > > > > > clocks are controlled through those registers. If they are inde=\n" - "ed configured\n" - "> > > > > > through the registers, the name is probably wrong and should be=\n" - " changed\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-kerne=\n" - "l\" in\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/\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/ -6842638c0b32a0386219dd68e576290ab6a8b537e0a805b9ac099dcb197c5bde +898530acaa744ab19f2c68f28dabb4ea1f3d6d35b629c76940742ed09eb68f23
diff --git a/a/1.txt b/N2/1.txt index 4b47760..cb05da2 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,79 +1,52 @@ Quoting Paul Osmialowski (2015-07-28 13:30:17) > Hi Mike, -> = - -> My trouble is that now I'm dealing with two conradictory opinions on how = - -> this driver should be written. The one you presented in your previous pos= -t = - -> assumes that there will be a header file with defines shared between the = - -> clock driver and DTS, also with clock gating details hidden behind some = - +> +> My trouble is that now I'm dealing with two conradictory opinions on how +> this driver should be written. The one you presented in your previous post +> assumes that there will be a header file with defines shared between the +> clock driver and DTS, also with clock gating details hidden behind some > additional level of indirection, e.g.: -> = - -> clocks =3D <&sim SIM_SCGC4_UART1_CLK>; -> = - -> Note that I've been through this at the very beginning, though the names = - +> +> clocks = <&sim SIM_SCGC4_UART1_CLK>; +> +> Note that I've been through this at the very beginning, though the names > I used have been bit different, e.g.: -> = - +> > #define KINETIS_CG_UART1 KINETIS_MKCG(3, 11) /* SIM_SCGC4[11] */ -> = - +> > This was rejected with a comment by Arnd: -> = - +> > Instead of using a triple indirection here, just put the tuples -> in the DT directly using #clock-cells=3D<2>, and get rid of both this +> in the DT directly using #clock-cells=<2>, and get rid of both this > header file and the dt-bindings/clock/kinetis-mcg.h file. Arnd, are you OK with my type of binding description now that I explained it with some examples? -> = - -> So I dropped all of these includes and started to use magic numbers (as = - -> you put it). Now I need to go one way or another or even go the third way= -: = - -> extend #clock-cells to <3> and address it like: <&sim parent_clock_id = - +> +> So I dropped all of these includes and started to use magic numbers (as +> you put it). Now I need to go one way or another or even go the third way: +> extend #clock-cells to <3> and address it like: <&sim parent_clock_id > scgc_register_number bit_index_number>. Paul, -From=20my understanding the DT folks do not like register-level or +>From my understanding the DT folks do not like register-level or bit-level details going into DT. It is better to handle the clock signals as abstract resources and link a provider and consumer with a simple phandle plus an index representing that abstract resource (i.e. the clock output signal). -> = - -> Reading your previous post I'm starting to feel that it would bring me = - -> closer to final acceptance if I stick to what you proposed in that post = - -> (I'm really grateful to you for writting so huge chunk of DTS for me!), s= -o = - +> +> Reading your previous post I'm starting to feel that it would bring me +> closer to final acceptance if I stick to what you proposed in that post +> (I'm really grateful to you for writting so huge chunk of DTS for me!), so > I'll probably adopt that. -> = - -> You're right about my "get things up and running" attitude - currently I = - -> want to develop things extensively (cover as much subsystems as = - -> possible) and then at some stage switch to intensive approach. This board = - -> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot = - +> +> You're right about my "get things up and running" attitude - currently I +> want to develop things extensively (cover as much subsystems as +> possible) and then at some stage switch to intensive approach. This board +> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot > of coding opportunity in this field in the future. I'm happy to take clock drivers and add my Reviewed-by to .dts files @@ -90,79 +63,58 @@ later if it needs to. Regards, Mike -> = - +> > Thanks, > Paul -> = - +> > On Tue, 28 Jul 2015, Michael Turquette wrote: -> = - +> > > 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 t= -hat 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. -> > = - +> > > > Did you look at the qcom clock binding and read the email where I > > detailed how that binding works? -> > = - +> > > > The point of that type of binding is to not shove per-clock data into > > DT, but instead to declare every clock controller IP block (e.g. the > > device) as well as every board-level clock (e.g. as osc that feeds into @@ -170,446 +122,317 @@ too) = > > create linkage between the clock providers and the clock consumers by > > using phandles + an index. Linux device drivers tap into this by using > > clk_get() and using the "clock-names" property from DT. -> > = - +> > > > Put another way: we mostly use DT to model "devices". That is open to > > interpretation for but for clock-related stuff we typically interpret > > the clock controller as the device, not the individual clock outputs > > coming out of the controller. -> > = - +> > > > Note that a clock controller IP block may be both a provider and a > > consumer. I/O controllers are a very common type of consumer (e.g. USB > > host controller, MMC controller, GPU, etc). -> > = - +> > > > Additionally, from my reading of the reference manual, mcgout is defined > > as: -> > = - +> > > > """ > > MCG output of either IRC, MCGFLLCLK MCGPLL0CLK, > > MCGPLL1CLK, or MCG's external reference clock that > > sources the core, system, bus, FlexBus, and flash clock. It is > > also an option for the debug trace clock. > > """ -> > = - +> > > > 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 > > fixed-dividers for the initial merge just to get things up and running > > if that is your strategy, but you'll need to revisit them later on when > > you need more flexible support for other boards. -> > = - +> > > > Again, I'm not sure why these clocks are enumerated in DT. Why not just > > enumerate your mcg clock controller and your sim clock controller? If > > you want to be a perfectionist then it appears that there is an osc > > clock controller upstream from the mcg controller as well ;-) -> > = - +> > > > It occurs to me that maybe you are trying to use fixed-factor clocks so > > that you can program a sane default rate? We use the > > assigned-clock-rates property for that. Note that this value is a > > 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 > > SoC, according to my reading of your technical reference manual. Just > > because you can model a few of its output clocks in dts does not mean > > 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 boa= -rd = - +> > +> > > +> > > 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 > > dtsi file. The board-level (osc) stuff can go into the individual board > > 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 +> > > > 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 configurat= -ion = - +> > > > +> > > > > +> > > > > 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 pa= -rent for = - -> > > > > core clock (CCLK) and peripheral clock (PCLK) is determined at ru= -n time by = - -> > > > > reading MCG registers, let me quote commit message from Emcraft g= -it repo: -> > > > > = - -> > > > > * Determine in run-time what oscillator module (OSC0 or OSC= -1) is 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 o= -sc0 +> > > > +> > > > 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 +> > > > +> > > > > * 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, bu= -t 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 CCL= -K and = - +> > > > > +> > > > > 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 th= -is; +> > > > +> > > > 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-speci= -fic -> > > > 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 RT= -C osc -> > > > can probably go in your chip-specific .dtsi as a fixed-rate clock s= -ince +> > > > +> > > > 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 n= -eed +> > > > +> > > > 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 M= -CG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; -> > > > clock-names =3D "core", "bus", "flexbus", "fl= -ash"; +> > > > +> > > > 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 clar= -ity. -> > > > The above is the only style of binding that I have been accepting f= -or -> > > > some time; first declare the clock controller and establish its reg= -ister -> > > > space, and then consumers can consume clocks by providing the phand= -le to +> > > > +> > > > 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 st= -uff +> > > > 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 +> > > > +> > > > 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 +> > > > +> > > > 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_manua= -l/K70P256M150SF3RM.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 mor= -e root = - -> > > > > clock, since it is also used directly (through CG reg. 1 bit 0) b= -y = - -> > > > > Freescale fec network device whose in-tree driver I'm trying to m= -ake = - +> > > > +> > > > > +> > > > > 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 th= -eir own -> > > > > >>>> nodes. Are they actually controlled by the same block? If th= -ey are -> > > > > >>>> just fixed, you can use the normal binding for fixed rate cl= -ocks +> > > > > >>>> 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 co= -nvincing. After -> > > > > >>> all, they all share the same I/O regs in order to read config= -uration. +> > > > > >>> 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 pr= -otecting -> > > > > >> the register access, but not by trying to group them artificia= -lly 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= - owns the -> > > > > > registers and that should be one node in DT, as Paul's first ve= -rsion 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 fix= -ed-rate -> > > > > > clocks are controlled through those registers. If they are inde= -ed configured -> > > > > > through the registers, the name is probably wrong and should be= - changed +> > > > > > 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-kerne= -l" in -> > > the body of a message to majordomo@vger.kernel.org +> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in +> > > 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/ -> > = - +> > > -- > 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 6121c06..fb603d5 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -7,121 +7,61 @@ "ref\0alpine.LNX.2.00.1507262211470.28341@localhost.localdomain\0" "ref\020150728160347.642.31950@quantum\0" "ref\0alpine.LNX.2.00.1507282201220.2619@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\0Wed, 29 Jul 2015 16:05:17 -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-28 13:30:17)\n" "> Hi Mike,\n" - "> =\n" - "\n" - "> My trouble is that now I'm dealing with two conradictory opinions on how =\n" - "\n" - "> this driver should be written. The one you presented in your previous pos=\n" - "t =\n" - "\n" - "> assumes that there will be a header file with defines shared between the =\n" - "\n" - "> clock driver and DTS, also with clock gating details hidden behind some =\n" - "\n" + "> \n" + "> My trouble is that now I'm dealing with two conradictory opinions on how \n" + "> this driver should be written. The one you presented in your previous post \n" + "> assumes that there will be a header file with defines shared between the \n" + "> clock driver and DTS, also with clock gating details hidden behind some \n" "> additional level of indirection, e.g.:\n" - "> =\n" - "\n" - "> clocks =3D <&sim SIM_SCGC4_UART1_CLK>;\n" - "> =\n" - "\n" - "> Note that I've been through this at the very beginning, though the names =\n" - "\n" + "> \n" + "> clocks = <&sim SIM_SCGC4_UART1_CLK>;\n" + "> \n" + "> Note that I've been through this at the very beginning, though the names \n" "> I used have been bit different, e.g.:\n" - "> =\n" - "\n" + "> \n" "> #define KINETIS_CG_UART1 KINETIS_MKCG(3, 11) /* SIM_SCGC4[11] */\n" - "> =\n" - "\n" + "> \n" "> This was rejected with a comment by Arnd:\n" - "> =\n" - "\n" + "> \n" "> Instead of using a triple indirection here, just put the tuples\n" - "> in the DT directly using #clock-cells=3D<2>, and get rid of both this\n" + "> in the DT directly using #clock-cells=<2>, and get rid of both this\n" "> header file and the dt-bindings/clock/kinetis-mcg.h file.\n" "\n" "Arnd, are you OK with my type of binding description now that I\n" "explained it with some examples?\n" "\n" - "> =\n" - "\n" - "> So I dropped all of these includes and started to use magic numbers (as =\n" - "\n" - "> you put it). Now I need to go one way or another or even go the third way=\n" - ": =\n" - "\n" - "> extend #clock-cells to <3> and address it like: <&sim parent_clock_id =\n" - "\n" + "> \n" + "> So I dropped all of these includes and started to use magic numbers (as \n" + "> you put it). Now I need to go one way or another or even go the third way: \n" + "> extend #clock-cells to <3> and address it like: <&sim parent_clock_id \n" "> scgc_register_number bit_index_number>.\n" "\n" "Paul,\n" "\n" - "From=20my understanding the DT folks do not like register-level or\n" + ">From my understanding the DT folks do not like register-level or\n" "bit-level details going into DT. It is better to handle the clock\n" "signals as abstract resources and link a provider and consumer with a\n" "simple phandle plus an index representing that abstract resource (i.e.\n" "the clock output signal).\n" "\n" - "> =\n" - "\n" - "> Reading your previous post I'm starting to feel that it would bring me =\n" - "\n" - "> closer to final acceptance if I stick to what you proposed in that post =\n" - "\n" - "> (I'm really grateful to you for writting so huge chunk of DTS for me!), s=\n" - "o =\n" - "\n" + "> \n" + "> Reading your previous post I'm starting to feel that it would bring me \n" + "> closer to final acceptance if I stick to what you proposed in that post \n" + "> (I'm really grateful to you for writting so huge chunk of DTS for me!), so \n" "> I'll probably adopt that.\n" - "> =\n" - "\n" - "> You're right about my \"get things up and running\" attitude - currently I =\n" - "\n" - "> want to develop things extensively (cover as much subsystems as =\n" - "\n" - "> possible) and then at some stage switch to intensive approach. This board =\n" - "\n" - "> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot =\n" - "\n" + "> \n" + "> You're right about my \"get things up and running\" attitude - currently I \n" + "> want to develop things extensively (cover as much subsystems as \n" + "> possible) and then at some stage switch to intensive approach. This board \n" + "> is a somewhat huge monster (for a Cortex-M4 SoC) and there will be a lot \n" "> of coding opportunity in this field in the future.\n" "\n" "I'm happy to take clock drivers and add my Reviewed-by to .dts files\n" @@ -138,79 +78,58 @@ "Regards,\n" "Mike\n" "\n" - "> =\n" - "\n" + "> \n" "> Thanks,\n" "> Paul\n" - "> =\n" - "\n" + "> \n" "> On Tue, 28 Jul 2015, Michael Turquette wrote:\n" - "> =\n" - "\n" + "> \n" "> > 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 t=\n" - "hat I =\n" - "\n" - "> > =\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" - "\n" + "> > \n" "> > No problem! And thanks for the quick turnaround on your patches so far.\n" - "> > =\n" - "\n" - "> > > know everything about my board, I can skip run-time discovery phase (=\n" - "note =\n" - "\n" - "> > > that the original driver was designed for other Kinetis-based boards =\n" - "too) =\n" - "\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" - "\n" + "> > \n" "> > I think this is a step backwards.\n" - "> > =\n" - "\n" + "> > \n" "> > Did you look at the qcom clock binding and read the email where I\n" "> > detailed how that binding works?\n" - "> > =\n" - "\n" + "> > \n" "> > The point of that type of binding is to not shove per-clock data into\n" "> > DT, but instead to declare every clock controller IP block (e.g. the\n" "> > device) as well as every board-level clock (e.g. as osc that feeds into\n" @@ -218,448 +137,319 @@ "> > create linkage between the clock providers and the clock consumers by\n" "> > using phandles + an index. Linux device drivers tap into this by using\n" "> > clk_get() and using the \"clock-names\" property from DT.\n" - "> > =\n" - "\n" + "> > \n" "> > Put another way: we mostly use DT to model \"devices\". That is open to\n" "> > interpretation for but for clock-related stuff we typically interpret\n" "> > the clock controller as the device, not the individual clock outputs\n" "> > coming out of the controller.\n" - "> > =\n" - "\n" + "> > \n" "> > Note that a clock controller IP block may be both a provider and a\n" "> > consumer. I/O controllers are a very common type of consumer (e.g. USB\n" "> > host controller, MMC controller, GPU, etc).\n" - "> > =\n" - "\n" + "> > \n" "> > Additionally, from my reading of the reference manual, mcgout is defined\n" "> > as:\n" - "> > =\n" - "\n" + "> > \n" "> > \"\"\"\n" "> > MCG output of either IRC, MCGFLLCLK MCGPLL0CLK,\n" "> > MCGPLL1CLK, or MCG's external reference clock that\n" "> > sources the core, system, bus, FlexBus, and flash clock. It is\n" "> > also an option for the debug trace clock.\n" "> > \"\"\"\n" - "> > =\n" - "\n" + "> > \n" "> > So why is it listed here as a fixed-factor clock? Is it not a\n" "> > 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" + "> > \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" - "> > =\n" - "\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" "> > fixed-dividers for the initial merge just to get things up and running\n" "> > if that is your strategy, but you'll need to revisit them later on when\n" "> > you need more flexible support for other boards.\n" - "> > =\n" - "\n" + "> > \n" "> > Again, I'm not sure why these clocks are enumerated in DT. Why not just\n" "> > enumerate your mcg clock controller and your sim clock controller? If\n" "> > you want to be a perfectionist then it appears that there is an osc\n" "> > clock controller upstream from the mcg controller as well ;-)\n" - "> > =\n" - "\n" + "> > \n" "> > It occurs to me that maybe you are trying to use fixed-factor clocks so\n" "> > that you can program a sane default rate? We use the\n" "> > assigned-clock-rates property for that. Note that this value is a\n" "> > property of some device which *consumes* the clock, not the clock\n" "> > controller or the clock output itself.\n" - "> > =\n" - "\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" - "\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" - "\n" - "> > > clock-names =3D \"ipg\";\n" - "> > > dmas =3D <&edma 0 2>;\n" - "> > > dma-names =3D \"rx\";\n" - "> > > status =3D \"disabled\";\n" + "> > \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" - "\n" + "> > \n" "> > I think the mcg should be required. The mcg is a real IP block on your\n" "> > SoC, according to my reading of your technical reference manual. Just\n" "> > because you can model a few of its output clocks in dts does not mean\n" "> > that you should.\n" - "> > =\n" - "\n" + "> > \n" "> > I did a quick grep and didn't find \"cmu\" anywhere in the reference\n" "> > manual.\n" - "> > =\n" - "\n" - "> > > =\n" - "\n" - "> > > I guess that the approach above would require split into soc-specific=\n" - " and =\n" - "\n" - "> > > board-specific part (as I said, dividers arrangement is something boa=\n" - "rd =\n" - "\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" - "\n" + "> > \n" "> > Splitting is good. Chip-specific stuff can go into the chip-specific\n" "> > dtsi file. The board-level (osc) stuff can go into the individual board\n" "> > files. The ultimate goal is to make it trivial to add new boards.\n" - "> > =\n" - "\n" + "> > \n" "> > Regards,\n" "> > Mike\n" - "> > =\n" - "\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 =\n" - "that\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 configurat=\n" - "ion =\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 pa=\n" - "rent for =\n" - "\n" - "> > > > > core clock (CCLK) and peripheral clock (PCLK) is determined at ru=\n" - "n time by =\n" - "\n" - "> > > > > reading MCG registers, let me quote commit message from Emcraft g=\n" - "it repo:\n" - "> > > > > =\n" - "\n" - "> > > > > * Determine in run-time what oscillator module (OSC0 or OSC=\n" - "1) is 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" - "> > > > According to [0] there are three options: a 32k RTC osc clock and o=\n" - "sc0\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 =\n" - "on all\n" - "> > > > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-=\n" - "SOM 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, bu=\n" - "t to go =\n" - "\n" - "> > > > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e=\n" - ".g. by =\n" - "\n" - "> > > > > U-boot. But that's too demanding for any potential users of this =\n" - "BSP. So =\n" - "\n" - "> > > > > let's asume that MCGOUTCLK is the root clock and a parent for CCL=\n" - "K and =\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" - "> > > > I'm confused. The point of device tree is to solve problems like th=\n" - "is;\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" - "> > > > OSC0 and OSC1 should each be a fixed-rate clock in your board-speci=\n" - "fic\n" - "> > > > TWR-K70F120M DTS (not a chip-specific file). They do not belong in =\n" - "the\n" - "> > > > cmu node, and they should use the \"fixed-clock\" binding. The 32k RT=\n" - "C osc\n" - "> > > > can probably go in your chip-specific .dtsi as a fixed-rate clock s=\n" - "ince\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" - "> > > > These three fixed-rate clocks are your root clock nodes. Customers =\n" - "only\n" - "> > > > need to worry about this if they spin a board, and then they will n=\n" - "eed\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 M=\n" - "CG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;\n" - "> > > > clock-names =3D \"core\", \"bus\", \"flexbus\", \"fl=\n" - "ash\";\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" - "> > > > I removed the interrupts and dma stuff from the uart0 node for clar=\n" - "ity.\n" - "> > > > The above is the only style of binding that I have been accepting f=\n" - "or\n" - "> > > > some time; first declare the clock controller and establish its reg=\n" - "ister\n" - "> > > > space, and then consumers can consume clocks by providing the phand=\n" - "le to\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" + "> > > > space, and then consumers can consume clocks by providing the phandle to\n" "> > > > the controller plus an offset corresponding to a unique clock. The\n" - "> > > > clock-names property makes it really easy to use with the clkdev st=\n" - "uff\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" - "> > > > Technically you could encode the same bits as sub-nodes of the mcg =\n" - "and\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" - "> > > > I think this means you can also get rid of kinetis_of_clk_get_name =\n" - "and\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_manua=\n" - "l/K70P256M150SF3RM.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 mor=\n" - "e root =\n" - "\n" - "> > > > > clock, since it is also used directly (through CG reg. 1 bit 0) b=\n" - "y =\n" - "\n" - "> > > > > Freescale fec network device whose in-tree driver I'm trying to m=\n" - "ake =\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 th=\n" - "eir own\n" - "> > > > > >>>> nodes. Are they actually controlled by the same block? If th=\n" - "ey are\n" - "> > > > > >>>> just fixed, you can use the normal binding for fixed rate cl=\n" - "ocks\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 co=\n" - "nvincing. After\n" - "> > > > > >>> all, they all share the same I/O regs in order to read config=\n" - "uration.\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=\n" - ". That's\n" - "> > > > > >> just a HW design decision and you need to deal with that by pr=\n" - "otecting\n" - "> > > > > >> the register access, but not by trying to group them artificia=\n" - "lly 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=\n" - " owns the\n" - "> > > > > > registers and that should be one node in DT, as Paul's first ve=\n" - "rsion 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 fix=\n" - "ed-rate\n" - "> > > > > > clocks are controlled through those registers. If they are inde=\n" - "ed configured\n" - "> > > > > > through the registers, the name is probably wrong and should be=\n" - " changed\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-kerne=\n" - "l\" in\n" - "> > > the body of a message to majordomo@vger.kernel.org\n" + "> > > To unsubscribe from this list: send the line \"unsubscribe linux-kernel\" in\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/\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/ -6842638c0b32a0386219dd68e576290ab6a8b537e0a805b9ac099dcb197c5bde +a9bc6d5bf52b5534f7ea08130d9fd6688090539a4dd0f6f87251253bc09bf688
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.