| Message ID | alpine.LNX.2.00.1507021206410.13210@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
On Thursday 02 July 2015 12:08:39 Paul Osmialowski wrote: > Nah, I've found this code hard to maintain. I'm attaching simplified > version. > Looks better to me, but of course needs full review from the clock maintainers. 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. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On Thu, 2 Jul 2015, Arnd Bergmann wrote: > I wonder if you could move out the fixed rate clocks into their own > nodes. Are they actually controlled by the same block? If they are > just fixed, you can use the normal binding for fixed rate clocks > and only describe the clocks that are related to the driver. > > Arnd > 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. Thanks, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2 Jul 2015, Paul Osmialowski wrote: > On Thu, 2 Jul 2015, Arnd Bergmann wrote: > > > I wonder if you could move out the fixed rate clocks into their own > > nodes. Are they actually controlled by the same block? If they are > > just fixed, you can use the normal binding for fixed rate clocks > > and only describe the clocks that are related to the driver. > > In my view having these clocks grouped together looks more convincing. After > all, they all share the same I/O regs in order to read configuration. The fact that they share a register is not making them a group. That's just a HW design decision and you need to deal with that by protecting the register access, but not by trying to group them artificially at the functional level. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote: > On Thu, 2 Jul 2015, Paul Osmialowski wrote: > > On Thu, 2 Jul 2015, Arnd Bergmann wrote: > > > > > I wonder if you could move out the fixed rate clocks into their own > > > nodes. Are they actually controlled by the same block? If they are > > > just fixed, you can use the normal binding for fixed rate clocks > > > and only describe the clocks that are related to the driver. > > > > In my view having these clocks grouped together looks more convincing. After > > all, they all share the same I/O regs in order to read configuration. > > The fact that they share a register is not making them a group. That's > just a HW design decision and you need to deal with that by protecting > the register access, but not by trying to group them artificially at > the functional level. I'd disagree with that: The clock controller is the device that owns the registers and that should be one node in DT, as Paul's first version does. The part I'm still struggling with is understanding how the fixed-rate clocks are controlled through those registers. If they are indeed configured through the registers, the name is probably wrong and should be changed to whatever kind of non-fixed clock this is. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Paul Osmialowski (2015-07-04 14:50:03) > Hi Arnd, > > I'm attaching excerpt from Kinetis reference manual that may make > situation clearer. Hi Paul, Can you please post the patch in the body of the email instead of an attachment? It makes it easier to review. Another small nitpick is that the $SUBJECT for this patch might be better off as something like: clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC At least it helps me find the patch I care about when skimming the series ;-) > > These MCG and SIM registers are used only to determine configuration > (clock fixed rates and clock signal origins) at run time. > > Namely, the real MCGOUTCLK source (in the middle) which is the parent for > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by > reading MCG registers, let me quote commit message from Emcraft git repo: > > * Determine in run-time what oscillator module (OSC0 or OSC1) is used > as clock source for the main PLL. According to [0] there are three options: a 32k RTC osc clock and osc0 both feed into a mux. You should model this 32k clock with the fixed-rate binding. > * When OSC1 is selected, assume its frequency to be 12 MHz on all > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and > TWR-K70F120M boards). > > In my .dts I'm trying to possibly follow real clock hierarchy, but to go > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by > U-boot. But that's too demanding for any potential users of this BSP. So > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and > PCLK. I'm confused. The point of device tree is to solve problems like this; i.e. board-specific differences such as different oscillator frequencies. OSC0 and OSC1 should each be a fixed-rate clock in your board-specific TWR-K70F120M DTS (not a chip-specific file). They do not belong in the cmu node, and they should use the "fixed-clock" binding. The 32k RTC osc can probably go in your chip-specific .dtsi as a fixed-rate clock since it appears to mandated in the reference manual[0]. These three fixed-rate clocks are your root clock nodes. Customers only need to worry about this if they spin a board, and then they will need to populate the frequencies of OSC0 and OSC1 in their board-specific .dts. Please break clk-kinetis.c into two files: drivers/clk/kinetis/clk-mcg.c drivers/clk/kinetis/clk-sim.c Below is what your binding/dts should look like: { osc0: clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <50000000>; }; osc1: clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <12000000>; }; rtc: clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>; }; soc: soc { mcg: clock-controller@40064000 { compatible = "fsl,kinetis-mcg"; clock-cells = <1>; reg = <0x40064000 0x14>; clocks = <&osc0>, <&osc1>, <&rtc>; clock-names = "osc0", "osc1", "rtc"; }; sim: clock-controller@40047000 { compatible = "fsl,kinetis-sim"; clock-cells = <1>; reg = <0x40047000 0x1100>; clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; clock-names = "core", "bus", "flexbus", "flash"; }; }; uart0: serial@4006a000 { compatible = "fsl,kinetis-lpuart"; reg = <0x4006a000 0x1000>; clocks = <&sim SIM_SCGC4_UART1_CLK>; clock-names = "gate"; }; I removed the interrupts and dma stuff from the uart0 node for clarity. The above is the only style of binding that I have been accepting for some time; first declare the clock controller and establish its register space, and then consumers can consume clocks by providing the phandle to the controller plus an offset corresponding to a unique clock. The clock-names property makes it really easy to use with the clkdev stuff (e.g. clk_get()). I've covered this before on the mailing list so here is a link describing how the qcom bindings do it in detail: http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum> Technically you could encode the same bits as sub-nodes of the mcg and sim nodes, but the shared header is how the magic happens with the driver so it's best to keep the clock controller binding small and light. I think this means you can also get rid of kinetis_of_clk_get_name and kinetis_clk_gate_get but my brain is tired so I'll leave that as an exercise to the reader. [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf Regards, Mike > > In my most recent version I added OSC0ERCLK explicitly as one more root > clock, since it is also used directly (through CG reg. 1 bit 0) by > Freescale fec network device whose in-tree driver I'm trying to make > usable for Kinetis. > > On Sat, 4 Jul 2015, Arnd Bergmann wrote: > > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote: > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote: > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote: > >>> > >>>> I wonder if you could move out the fixed rate clocks into their own > >>>> nodes. Are they actually controlled by the same block? If they are > >>>> just fixed, you can use the normal binding for fixed rate clocks > >>>> and only describe the clocks that are related to the driver. > >>> > >>> In my view having these clocks grouped together looks more convincing. After > >>> all, they all share the same I/O regs in order to read configuration. > >> > >> The fact that they share a register is not making them a group. That's > >> just a HW design decision and you need to deal with that by protecting > >> the register access, but not by trying to group them artificially at > >> the functional level. > > > > I'd disagree with that: The clock controller is the device that owns the > > registers and that should be one node in DT, as Paul's first version does. > > > > The part I'm still struggling with is understanding how the fixed-rate > > clocks are controlled through those registers. If they are indeed configured > > through the registers, the name is probably wrong and should be changed > > to whatever kind of non-fixed clock this is. > > > > Arnd > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mike,
Thank you for spending time on this and pointing me into the right
direction. I'm wondering about going even further with it. Assuming that I
know everything about my board, I can skip run-time discovery phase (note
that the original driver was designed for other Kinetis-based boards too)
and move everything into DTS, somewhat like this:
/ {
osc0: clock {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <50000000>;
};
osc1: clock {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <12000000>;
};
rtc: clock {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <32768>;
};
mcgout: clock {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
clocks = <&osc0>;
clock-mult = <12>;
clock-div = <5>;
};
core: clock {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
clocks = <&mcgout>;
clock-mult = <1>;
clock-div = <1>;
};
bus: clock {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
clocks = <&mcgout>;
clock-mult = <1>;
clock-div = <2>;
};
soc {
cmu@0x40047000 {
compatible = "fsl,kinetis-gate-clock";
reg = <0x40047000 0x1100>;
mcg_core_gate: clock-gate {
clocks = <&core>;
#clock-cells = <2>;
};
mcg_bus_gate: clock-gate {
clocks = <&bus>;
#clock-cells = <2>;
};
osc0_erclk_gate: clock-gate {
clocks = <&osc0>;
#clock-cells = <2>;
};
};
uart0: serial@4006a000 {
compatible = "fsl,kinetis-lpuart";
reg = <0x4006a000 0x1000>;
interrupts = <45>, <46>;
interrupt-names = "uart-stat", "uart-err";
clocks = <&mcg_core_gate 3 10>;
clock-names = "ipg";
dmas = <&edma 0 2>;
dma-names = "rx";
status = "disabled";
};
};
};
As you can see, mcg part is not required anymore.
I guess that the approach above would require split into soc-specific and
board-specific part (as I said, dividers arrangement is something board
specific), but I wonder what you thing about this proposal.
Thanks,
Paul
On Thu, 23 Jul 2015, Michael Turquette wrote:
> Quoting Paul Osmialowski (2015-07-04 14:50:03)
> > Hi Arnd,
> >
> > I'm attaching excerpt from Kinetis reference manual that may make
> > situation clearer.
>
> Hi Paul,
>
> Can you please post the patch in the body of the email instead of an
> attachment? It makes it easier to review. Another small nitpick is that
> the $SUBJECT for this patch might be better off as something like:
>
> clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC
>
> At least it helps me find the patch I care about when skimming the
> series ;-)
>
> >
> > These MCG and SIM registers are used only to determine configuration
> > (clock fixed rates and clock signal origins) at run time.
> >
> > Namely, the real MCGOUTCLK source (in the middle) which is the parent for
> > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by
> > reading MCG registers, let me quote commit message from Emcraft git repo:
> >
> > * Determine in run-time what oscillator module (OSC0 or OSC1) is used
> > as clock source for the main PLL.
>
> According to [0] there are three options: a 32k RTC osc clock and osc0
> both feed into a mux. You should model this 32k clock with the
> fixed-rate binding.
>
> > * When OSC1 is selected, assume its frequency to be 12 MHz on all
> > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and
> > TWR-K70F120M boards).
> >
> > In my .dts I'm trying to possibly follow real clock hierarchy, but to go
> > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by
> > U-boot. But that's too demanding for any potential users of this BSP. So
> > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and
> > PCLK.
>
> I'm confused. The point of device tree is to solve problems like this;
> i.e. board-specific differences such as different oscillator
> frequencies.
>
> OSC0 and OSC1 should each be a fixed-rate clock in your board-specific
> TWR-K70F120M DTS (not a chip-specific file). They do not belong in the
> cmu node, and they should use the "fixed-clock" binding. The 32k RTC osc
> can probably go in your chip-specific .dtsi as a fixed-rate clock since
> it appears to mandated in the reference manual[0].
>
> These three fixed-rate clocks are your root clock nodes. Customers only
> need to worry about this if they spin a board, and then they will need
> to populate the frequencies of OSC0 and OSC1 in their board-specific
> .dts.
>
> Please break clk-kinetis.c into two files:
> drivers/clk/kinetis/clk-mcg.c
> drivers/clk/kinetis/clk-sim.c
>
> Below is what your binding/dts should look like:
>
> {
> osc0: clock {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <50000000>;
> };
>
> osc1: clock {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <12000000>;
> };
>
> rtc: clock {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <32768>;
> };
>
> soc: soc {
> mcg: clock-controller@40064000 {
> compatible = "fsl,kinetis-mcg";
> clock-cells = <1>;
> reg = <0x40064000 0x14>;
> clocks = <&osc0>, <&osc1>, <&rtc>;
> clock-names = "osc0", "osc1", "rtc";
> };
>
> sim: clock-controller@40047000 {
> compatible = "fsl,kinetis-sim";
> clock-cells = <1>;
> reg = <0x40047000 0x1100>;
> clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;
> clock-names = "core", "bus", "flexbus", "flash";
> };
> };
>
> uart0: serial@4006a000 {
> compatible = "fsl,kinetis-lpuart";
> reg = <0x4006a000 0x1000>;
> clocks = <&sim SIM_SCGC4_UART1_CLK>;
> clock-names = "gate";
> };
>
> I removed the interrupts and dma stuff from the uart0 node for clarity.
> The above is the only style of binding that I have been accepting for
> some time; first declare the clock controller and establish its register
> space, and then consumers can consume clocks by providing the phandle to
> the controller plus an offset corresponding to a unique clock. The
> clock-names property makes it really easy to use with the clkdev stuff
> (e.g. clk_get()).
>
> I've covered this before on the mailing list so here is a link
> describing how the qcom bindings do it in detail:
>
> http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum>
>
> Technically you could encode the same bits as sub-nodes of the mcg and
> sim nodes, but the shared header is how the magic happens with the
> driver so it's best to keep the clock controller binding small and
> light.
>
> I think this means you can also get rid of kinetis_of_clk_get_name and
> kinetis_clk_gate_get but my brain is tired so I'll leave that as an
> exercise to the reader.
>
> [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf
>
> Regards,
> Mike
>
> >
> > In my most recent version I added OSC0ERCLK explicitly as one more root
> > clock, since it is also used directly (through CG reg. 1 bit 0) by
> > Freescale fec network device whose in-tree driver I'm trying to make
> > usable for Kinetis.
> >
> > On Sat, 4 Jul 2015, Arnd Bergmann wrote:
> >
> > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:
> > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote:
> > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote:
> > >>>
> > >>>> I wonder if you could move out the fixed rate clocks into their own
> > >>>> nodes. Are they actually controlled by the same block? If they are
> > >>>> just fixed, you can use the normal binding for fixed rate clocks
> > >>>> and only describe the clocks that are related to the driver.
> > >>>
> > >>> In my view having these clocks grouped together looks more convincing. After
> > >>> all, they all share the same I/O regs in order to read configuration.
> > >>
> > >> The fact that they share a register is not making them a group. That's
> > >> just a HW design decision and you need to deal with that by protecting
> > >> the register access, but not by trying to group them artificially at
> > >> the functional level.
> > >
> > > I'd disagree with that: The clock controller is the device that owns the
> > > registers and that should be one node in DT, as Paul's first version does.
> > >
> > > The part I'm still struggling with is understanding how the fixed-rate
> > > clocks are controlled through those registers. If they are indeed configured
> > > through the registers, the name is probably wrong and should be changed
> > > to whatever kind of non-fixed clock this is.
> > >
> > > Arnd
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 = <&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=<2>, and get rid of both this header file and the dt-bindings/clock/kinetis-mcg.h file. 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>. 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 of coding opportunity in this field in the future. 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 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) > > and move everything into DTS, somewhat like this: > > > > / { > > osc0: clock { > > compatible = "fixed-clock"; > > #clock-cells = <0>; > > clock-frequency = <50000000>; > > }; > > > > osc1: clock { > > compatible = "fixed-clock"; > > #clock-cells = <0>; > > clock-frequency = <12000000>; > > }; > > > > rtc: clock { > > compatible = "fixed-clock"; > > #clock-cells = <0>; > > clock-frequency = <32768>; > > }; > > > > mcgout: clock { > > compatible = "fixed-factor-clock"; > > #clock-cells = <0>; > > clocks = <&osc0>; > > clock-mult = <12>; > > clock-div = <5>; > > }; > > 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 > your mcu). Once these "clock providers" are enumerated in DT, then we > 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 = "fixed-factor-clock"; > > #clock-cells = <0>; > > clocks = <&mcgout>; > > clock-mult = <1>; > > clock-div = <1>; > > }; > > > > bus: clock { > > compatible = "fixed-factor-clock"; > > #clock-cells = <0>; > > clocks = <&mcgout>; > > clock-mult = <1>; > > clock-div = <2>; > > 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 = "fsl,kinetis-gate-clock"; > > reg = <0x40047000 0x1100>; > > > > mcg_core_gate: clock-gate { > > clocks = <&core>; > > #clock-cells = <2>; > > }; > > > > mcg_bus_gate: clock-gate { > > clocks = <&bus>; > > #clock-cells = <2>; > > }; > > > > osc0_erclk_gate: clock-gate { > > clocks = <&osc0>; > > #clock-cells = <2>; > > }; > > }; > > > > uart0: serial@4006a000 { > > compatible = "fsl,kinetis-lpuart"; > > reg = <0x4006a000 0x1000>; > > interrupts = <45>, <46>; > > interrupt-names = "uart-stat", "uart-err"; > > clocks = <&mcg_core_gate 3 10>; > > 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 = "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 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 > > > > situation clearer. > > > > > > Hi Paul, > > > > > > Can you please post the patch in the body of the email instead of an > > > attachment? It makes it easier to review. Another small nitpick is that > > > the $SUBJECT for this patch might be better off as something like: > > > > > > clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC > > > > > > At least it helps me find the patch I care about when skimming the > > > series ;-) > > > > > > > > > > > These MCG and SIM registers are used only to determine configuration > > > > (clock fixed rates and clock signal origins) at run time. > > > > > > > > Namely, the real MCGOUTCLK source (in the middle) which is the parent for > > > > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by > > > > reading MCG registers, let me quote commit message from Emcraft git repo: > > > > > > > > * Determine in run-time what oscillator module (OSC0 or OSC1) is used > > > > as clock source for the main PLL. > > > > > > According to [0] there are three options: a 32k RTC osc clock and osc0 > > > both feed into a mux. You should model this 32k clock with the > > > fixed-rate binding. > > > > > > > * When OSC1 is selected, assume its frequency to be 12 MHz on all > > > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and > > > > TWR-K70F120M boards). > > > > > > > > In my .dts I'm trying to possibly follow real clock hierarchy, but to go > > > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by > > > > U-boot. But that's too demanding for any potential users of this BSP. So > > > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and > > > > PCLK. > > > > > > I'm confused. The point of device tree is to solve problems like this; > > > i.e. board-specific differences such as different oscillator > > > frequencies. > > > > > > OSC0 and OSC1 should each be a fixed-rate clock in your board-specific > > > TWR-K70F120M DTS (not a chip-specific file). They do not belong in the > > > cmu node, and they should use the "fixed-clock" binding. The 32k RTC osc > > > can probably go in your chip-specific .dtsi as a fixed-rate clock since > > > it appears to mandated in the reference manual[0]. > > > > > > These three fixed-rate clocks are your root clock nodes. Customers only > > > need to worry about this if they spin a board, and then they will need > > > to populate the frequencies of OSC0 and OSC1 in their board-specific > > > .dts. > > > > > > Please break clk-kinetis.c into two files: > > > drivers/clk/kinetis/clk-mcg.c > > > drivers/clk/kinetis/clk-sim.c > > > > > > Below is what your binding/dts should look like: > > > > > > { > > > osc0: clock { > > > compatible = "fixed-clock"; > > > #clock-cells = <0>; > > > clock-frequency = <50000000>; > > > }; > > > > > > osc1: clock { > > > compatible = "fixed-clock"; > > > #clock-cells = <0>; > > > clock-frequency = <12000000>; > > > }; > > > > > > rtc: clock { > > > compatible = "fixed-clock"; > > > #clock-cells = <0>; > > > clock-frequency = <32768>; > > > }; > > > > > > soc: soc { > > > mcg: clock-controller@40064000 { > > > compatible = "fsl,kinetis-mcg"; > > > clock-cells = <1>; > > > reg = <0x40064000 0x14>; > > > clocks = <&osc0>, <&osc1>, <&rtc>; > > > clock-names = "osc0", "osc1", "rtc"; > > > }; > > > > > > sim: clock-controller@40047000 { > > > compatible = "fsl,kinetis-sim"; > > > clock-cells = <1>; > > > reg = <0x40047000 0x1100>; > > > clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; > > > clock-names = "core", "bus", "flexbus", "flash"; > > > }; > > > }; > > > > > > uart0: serial@4006a000 { > > > compatible = "fsl,kinetis-lpuart"; > > > reg = <0x4006a000 0x1000>; > > > clocks = <&sim SIM_SCGC4_UART1_CLK>; > > > clock-names = "gate"; > > > }; > > > > > > I removed the interrupts and dma stuff from the uart0 node for clarity. > > > The above is the only style of binding that I have been accepting for > > > some time; first declare the clock controller and establish its register > > > space, and then consumers can consume clocks by providing the phandle to > > > the controller plus an offset corresponding to a unique clock. The > > > clock-names property makes it really easy to use with the clkdev stuff > > > (e.g. clk_get()). > > > > > > I've covered this before on the mailing list so here is a link > > > describing how the qcom bindings do it in detail: > > > > > > http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum> > > > > > > Technically you could encode the same bits as sub-nodes of the mcg and > > > sim nodes, but the shared header is how the magic happens with the > > > driver so it's best to keep the clock controller binding small and > > > light. > > > > > > I think this means you can also get rid of kinetis_of_clk_get_name and > > > kinetis_clk_gate_get but my brain is tired so I'll leave that as an > > > exercise to the reader. > > > > > > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf > > > > > > Regards, > > > Mike > > > > > > > > > > > In my most recent version I added OSC0ERCLK explicitly as one more root > > > > clock, since it is also used directly (through CG reg. 1 bit 0) by > > > > Freescale fec network device whose in-tree driver I'm trying to make > > > > usable for Kinetis. > > > > > > > > On Sat, 4 Jul 2015, Arnd Bergmann wrote: > > > > > > > > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote: > > > > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote: > > > > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote: > > > > >>> > > > > >>>> I wonder if you could move out the fixed rate clocks into their own > > > > >>>> nodes. Are they actually controlled by the same block? If they are > > > > >>>> just fixed, you can use the normal binding for fixed rate clocks > > > > >>>> and only describe the clocks that are related to the driver. > > > > >>> > > > > >>> In my view having these clocks grouped together looks more convincing. After > > > > >>> all, they all share the same I/O regs in order to read configuration. > > > > >> > > > > >> The fact that they share a register is not making them a group. That's > > > > >> just a HW design decision and you need to deal with that by protecting > > > > >> the register access, but not by trying to group them artificially at > > > > >> the functional level. > > > > > > > > > > I'd disagree with that: The clock controller is the device that owns the > > > > > registers and that should be one node in DT, as Paul's first version does. > > > > > > > > > > The part I'm still struggling with is understanding how the fixed-rate > > > > > clocks are controlled through those registers. If they are indeed configured > > > > > through the registers, the name is probably wrong and should be changed > > > > > to whatever kind of non-fixed clock this is. > > > > > > > > > > Arnd > > > > > > > > > > > > > _______________________________________________ > > > > linux-arm-kernel mailing list > > > > linux-arm-kernel@lists.infradead.org > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mike,
I encountered some trouble while I tried to implement code fitting to DTS
that you proposed. SIM_CLKDIVx are SIM device registers, not MCG.
Therefore, in MCG device code I won't be able to figure out clock rate
for outputed clocks unles I try to access SIM registers from MCG driver -
that's what we wanted to avoid. Looks like it's the mcgoutclk that we want
to expose not core, bus and the others:
soc {
mcg: clock-controller@40064000 {
compatible = "fsl,kinetis-mcg";
clock-cells = <0>;
reg = <0x40064000 0x14>;
clocks = <&osc0>, <&osc1>, <&rtc>;
clock-names = "osc0", "osc1", "rtc";
};
sim: clock-controller@40047000 {
compatible = "fsl,kinetis-sim";
clock-cells = <1>;
reg = <0x40047000 0x1100>;
clocks = <&mcg>,
<&osc0>;
clock-names = "mcgoutclk", "enet";
};
uart0: serial@4006a000 {
compatible = "fsl,kinetis-lpuart";
reg = <0x4006a000 0x1000>;
interrupts = <45>, <46>;
interrupt-names = "uart-stat", "uart-err";
clocks = <&sim SIM_SCGC4_UART0_CLK>;
clock-names = "ipg";
dmas = <&edma 0 2>;
dma-names = "rx";
status = "disabled";
};
};
Now fsl,kinetis-sim device will be responsible for clock gating and it
will know about core, bus, flexbus and flash clocks only internally -
none of them are exposed.
Also note that fec (ethernet device) driver is connected directly to osc0
(though clock gate, you can see this CG attached to osc0 on the diagram
too) - to control this gate I need to access SIM device registers, so it
should be covered by the same fsl,kinetis-sim driver.
On Wed, 29 Jul 2015, Michael Turquette wrote:
> 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 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 = <&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=<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
> > scgc_register_number bit_index_number>.
>
> Paul,
>
> >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!), 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
> > of coding opportunity in this field in the future.
>
> I'm happy to take clock drivers and add my Reviewed-by to .dts files
> that make use of fixed-rate and fixed-factor clocks as an interim
> solution. Of course it will be best to get The Real Thing merged
> upstream asap, but this is something I've done before to help get new
> platform upstream before and I'm fine to do it again.
>
> With that said, Devicetree bindings are allegedly a stable ABI that
> cannot be broken. So let's make sure that any Kinetis clock binding
> description is in good shape before merging it. The rest can follow on
> 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 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)
> > > > and move everything into DTS, somewhat like this:
> > > >
> > > > / {
> > > > osc0: clock {
> > > > compatible = "fixed-clock";
> > > > #clock-cells = <0>;
> > > > clock-frequency = <50000000>;
> > > > };
> > > >
> > > > osc1: clock {
> > > > compatible = "fixed-clock";
> > > > #clock-cells = <0>;
> > > > clock-frequency = <12000000>;
> > > > };
> > > >
> > > > rtc: clock {
> > > > compatible = "fixed-clock";
> > > > #clock-cells = <0>;
> > > > clock-frequency = <32768>;
> > > > };
> > > >
> > > > mcgout: clock {
> > > > compatible = "fixed-factor-clock";
> > > > #clock-cells = <0>;
> > > > clocks = <&osc0>;
> > > > clock-mult = <12>;
> > > > clock-div = <5>;
> > > > };
> > >
> > > 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
> > > your mcu). Once these "clock providers" are enumerated in DT, then we
> > > 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 = "fixed-factor-clock";
> > > > #clock-cells = <0>;
> > > > clocks = <&mcgout>;
> > > > clock-mult = <1>;
> > > > clock-div = <1>;
> > > > };
> > > >
> > > > bus: clock {
> > > > compatible = "fixed-factor-clock";
> > > > #clock-cells = <0>;
> > > > clocks = <&mcgout>;
> > > > clock-mult = <1>;
> > > > clock-div = <2>;
> > >
> > > 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 = "fsl,kinetis-gate-clock";
> > > > reg = <0x40047000 0x1100>;
> > > >
> > > > mcg_core_gate: clock-gate {
> > > > clocks = <&core>;
> > > > #clock-cells = <2>;
> > > > };
> > > >
> > > > mcg_bus_gate: clock-gate {
> > > > clocks = <&bus>;
> > > > #clock-cells = <2>;
> > > > };
> > > >
> > > > osc0_erclk_gate: clock-gate {
> > > > clocks = <&osc0>;
> > > > #clock-cells = <2>;
> > > > };
> > > > };
> > > >
> > > > uart0: serial@4006a000 {
> > > > compatible = "fsl,kinetis-lpuart";
> > > > reg = <0x4006a000 0x1000>;
> > > > interrupts = <45>, <46>;
> > > > interrupt-names = "uart-stat", "uart-err";
> > > > clocks = <&mcg_core_gate 3 10>;
> > >
> > > 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 = "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 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
> > > > > > situation clearer.
> > > > >
> > > > > Hi Paul,
> > > > >
> > > > > Can you please post the patch in the body of the email instead of an
> > > > > attachment? It makes it easier to review. Another small nitpick is that
> > > > > the $SUBJECT for this patch might be better off as something like:
> > > > >
> > > > > clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC
> > > > >
> > > > > At least it helps me find the patch I care about when skimming the
> > > > > series ;-)
> > > > >
> > > > > >
> > > > > > These MCG and SIM registers are used only to determine configuration
> > > > > > (clock fixed rates and clock signal origins) at run time.
> > > > > >
> > > > > > Namely, the real MCGOUTCLK source (in the middle) which is the parent for
> > > > > > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by
> > > > > > reading MCG registers, let me quote commit message from Emcraft git repo:
> > > > > >
> > > > > > * Determine in run-time what oscillator module (OSC0 or OSC1) is used
> > > > > > as clock source for the main PLL.
> > > > >
> > > > > According to [0] there are three options: a 32k RTC osc clock and osc0
> > > > > both feed into a mux. You should model this 32k clock with the
> > > > > fixed-rate binding.
> > > > >
> > > > > > * When OSC1 is selected, assume its frequency to be 12 MHz on all
> > > > > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and
> > > > > > TWR-K70F120M boards).
> > > > > >
> > > > > > In my .dts I'm trying to possibly follow real clock hierarchy, but to go
> > > > > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by
> > > > > > U-boot. But that's too demanding for any potential users of this BSP. So
> > > > > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and
> > > > > > PCLK.
> > > > >
> > > > > I'm confused. The point of device tree is to solve problems like this;
> > > > > i.e. board-specific differences such as different oscillator
> > > > > frequencies.
> > > > >
> > > > > OSC0 and OSC1 should each be a fixed-rate clock in your board-specific
> > > > > TWR-K70F120M DTS (not a chip-specific file). They do not belong in the
> > > > > cmu node, and they should use the "fixed-clock" binding. The 32k RTC osc
> > > > > can probably go in your chip-specific .dtsi as a fixed-rate clock since
> > > > > it appears to mandated in the reference manual[0].
> > > > >
> > > > > These three fixed-rate clocks are your root clock nodes. Customers only
> > > > > need to worry about this if they spin a board, and then they will need
> > > > > to populate the frequencies of OSC0 and OSC1 in their board-specific
> > > > > .dts.
> > > > >
> > > > > Please break clk-kinetis.c into two files:
> > > > > drivers/clk/kinetis/clk-mcg.c
> > > > > drivers/clk/kinetis/clk-sim.c
> > > > >
> > > > > Below is what your binding/dts should look like:
> > > > >
> > > > > {
> > > > > osc0: clock {
> > > > > compatible = "fixed-clock";
> > > > > #clock-cells = <0>;
> > > > > clock-frequency = <50000000>;
> > > > > };
> > > > >
> > > > > osc1: clock {
> > > > > compatible = "fixed-clock";
> > > > > #clock-cells = <0>;
> > > > > clock-frequency = <12000000>;
> > > > > };
> > > > >
> > > > > rtc: clock {
> > > > > compatible = "fixed-clock";
> > > > > #clock-cells = <0>;
> > > > > clock-frequency = <32768>;
> > > > > };
> > > > >
> > > > > soc: soc {
> > > > > mcg: clock-controller@40064000 {
> > > > > compatible = "fsl,kinetis-mcg";
> > > > > clock-cells = <1>;
> > > > > reg = <0x40064000 0x14>;
> > > > > clocks = <&osc0>, <&osc1>, <&rtc>;
> > > > > clock-names = "osc0", "osc1", "rtc";
> > > > > };
> > > > >
> > > > > sim: clock-controller@40047000 {
> > > > > compatible = "fsl,kinetis-sim";
> > > > > clock-cells = <1>;
> > > > > reg = <0x40047000 0x1100>;
> > > > > clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;
> > > > > clock-names = "core", "bus", "flexbus", "flash";
> > > > > };
> > > > > };
> > > > >
> > > > > uart0: serial@4006a000 {
> > > > > compatible = "fsl,kinetis-lpuart";
> > > > > reg = <0x4006a000 0x1000>;
> > > > > clocks = <&sim SIM_SCGC4_UART1_CLK>;
> > > > > clock-names = "gate";
> > > > > };
> > > > >
> > > > > I removed the interrupts and dma stuff from the uart0 node for clarity.
> > > > > The above is the only style of binding that I have been accepting for
> > > > > some time; first declare the clock controller and establish its register
> > > > > space, and then consumers can consume clocks by providing the phandle to
> > > > > the controller plus an offset corresponding to a unique clock. The
> > > > > clock-names property makes it really easy to use with the clkdev stuff
> > > > > (e.g. clk_get()).
> > > > >
> > > > > I've covered this before on the mailing list so here is a link
> > > > > describing how the qcom bindings do it in detail:
> > > > >
> > > > > http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum>
> > > > >
> > > > > Technically you could encode the same bits as sub-nodes of the mcg and
> > > > > sim nodes, but the shared header is how the magic happens with the
> > > > > driver so it's best to keep the clock controller binding small and
> > > > > light.
> > > > >
> > > > > I think this means you can also get rid of kinetis_of_clk_get_name and
> > > > > kinetis_clk_gate_get but my brain is tired so I'll leave that as an
> > > > > exercise to the reader.
> > > > >
> > > > > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf
> > > > >
> > > > > Regards,
> > > > > Mike
> > > > >
> > > > > >
> > > > > > In my most recent version I added OSC0ERCLK explicitly as one more root
> > > > > > clock, since it is also used directly (through CG reg. 1 bit 0) by
> > > > > > Freescale fec network device whose in-tree driver I'm trying to make
> > > > > > usable for Kinetis.
> > > > > >
> > > > > > On Sat, 4 Jul 2015, Arnd Bergmann wrote:
> > > > > >
> > > > > > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:
> > > > > > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote:
> > > > > > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote:
> > > > > > >>>
> > > > > > >>>> I wonder if you could move out the fixed rate clocks into their own
> > > > > > >>>> nodes. Are they actually controlled by the same block? If they are
> > > > > > >>>> just fixed, you can use the normal binding for fixed rate clocks
> > > > > > >>>> and only describe the clocks that are related to the driver.
> > > > > > >>>
> > > > > > >>> In my view having these clocks grouped together looks more convincing. After
> > > > > > >>> all, they all share the same I/O regs in order to read configuration.
> > > > > > >>
> > > > > > >> The fact that they share a register is not making them a group. That's
> > > > > > >> just a HW design decision and you need to deal with that by protecting
> > > > > > >> the register access, but not by trying to group them artificially at
> > > > > > >> the functional level.
> > > > > > >
> > > > > > > I'd disagree with that: The clock controller is the device that owns the
> > > > > > > registers and that should be one node in DT, as Paul's first version does.
> > > > > > >
> > > > > > > The part I'm still struggling with is understanding how the fixed-rate
> > > > > > > clocks are controlled through those registers. If they are indeed configured
> > > > > > > through the registers, the name is probably wrong and should be changed
> > > > > > > to whatever kind of non-fixed clock this is.
> > > > > > >
> > > > > > > Arnd
> > > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > linux-arm-kernel mailing list
> > > > > > linux-arm-kernel@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at http://www.tux.org/lkml/
> > >
> > --
> > 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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mike, On Fri, 31 Jul 2015, Michael Turquette wrote: > Quoting Paul Osmialowski (2015-07-30 14:40:48) > > Hi Mike, > > > > I encountered some trouble while I tried to implement code fitting to DTS > > that you proposed. SIM_CLKDIVx are SIM device registers, not MCG. > > Therefore, in MCG device code I won't be able to figure out clock rate > > for outputed clocks unles I try to access SIM registers from MCG driver - > > Right, the MCG driver will only know the rates of the clocks that it > outputs, namely: > > MCGIRCLK > MCGFFCLK > MCGOUTCLK > MCGFLLCLK > MCGPLL0CLK > MCGPLL1CLK > > The naming scheme in the TRM is a bit unfortunate; it looks like some of > the clock outputs coming out of SIM also retain the MCG* names. I wonder if I really need to implement support for all of these clocks while to run Linux on this board reference 2.6 kernel uses only MCGOUTCLK (and for this, it never considers FLL as a source). I can also provide support for MCGPLL0CLK and MCGPLL1CLK as well since their rates are computed as a byproduct while determining rate of MCGOUTCLK. I may define constants for all of them in related header file, but I'm thinking about leaving most of implementations as TBD - otherwise I doubt I'll be ever able to test that my driver fully works. > > Why is clock-cells zero? I think the MCG block outputs all of the clocks > that I listed above? As such they should be addressable as <&mcg N> by > any downstream users. > Ok, will be 1, to distinguish between MCGOUTCLK, MCGPLL0CLK, MCGPLL1CLK. > > reg = <0x40064000 0x14>; > > clocks = <&osc0>, <&osc1>, <&rtc>; > > clock-names = "osc0", "osc1", "rtc"; > > Are the oscN and rtc clocks modeled as fixed-rate clocks in DT? > Yes, I skipped the part above soc during copy-paste. > It looks like the SIM block should also consume: > > MCGFLLCLK > MCGIRCLK > MCGFFCLK > MCGPLL0CLK > MCGPLL1CLK > OSC0ERCLK > OSC1ERCLK > OSC032KCLK > RTCCLK (I made that name up) > > The mcg clocks should come out of the mcg node. It looks like you > already modeled oscN and rtc clocks, which is great. There are some > gates after these clocks (e.g. OSC0ERCLK). I wonder if those gates are > actually in the osc IP block or if they actually live in SIM. More on > that below... > > Well they certainly can be exposed if you need them to be. If a device > driver wants to get the core clock and set it's rate then it should be > able to with something like <&sim CORE_CLK>. > Ok, the user of the clock does not relly need to know whether it is clock device or a clock gate - fortunately, I already learnt how to implement such thing. > > > > Also note that fec (ethernet device) driver is connected directly to osc0 > > (though clock gate, you can see this CG attached to osc0 on the diagram > > too) - to control this gate I need to access SIM device registers, so it > > should be covered by the same fsl,kinetis-sim driver. > > Do you mean the SIM_SOPT2 register? It looks like the connection is not > "direct", as the osc0 signal feeds into the SIM block and the gate there > controls it. Perhaps the diagram is incorrect by placing the CG block > inside osc0? It looks like you correctly modeled this relationship in > the sim node. In order to enable network device, bit 0 of SIM_SCGC2 must be set for RMII mode to work, and according to what I see on the page 228 of the reference manual, it is osc0 clock behind this gate. In the context of network device, I would need to read SIM_SOPT2 to determine clock source for IEEE 1588 timestamp while implementing PTP support - which currently I don't have in my plans, network device can do without it. Thanks, Paul > > Besides my annoying questions above this binding is starting to shape up > very well. Thanks for your patience! > > Regards, > Mike > > > > > On Wed, 29 Jul 2015, Michael Turquette wrote: > > > > > 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 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 = <&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=<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 > > > > scgc_register_number bit_index_number>. > > > > > > Paul, > > > > > > >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!), 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 > > > > of coding opportunity in this field in the future. > > > > > > I'm happy to take clock drivers and add my Reviewed-by to .dts files > > > that make use of fixed-rate and fixed-factor clocks as an interim > > > solution. Of course it will be best to get The Real Thing merged > > > upstream asap, but this is something I've done before to help get new > > > platform upstream before and I'm fine to do it again. > > > > > > With that said, Devicetree bindings are allegedly a stable ABI that > > > cannot be broken. So let's make sure that any Kinetis clock binding > > > description is in good shape before merging it. The rest can follow on > > > 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 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) > > > > > > and move everything into DTS, somewhat like this: > > > > > > > > > > > > / { > > > > > > osc0: clock { > > > > > > compatible = "fixed-clock"; > > > > > > #clock-cells = <0>; > > > > > > clock-frequency = <50000000>; > > > > > > }; > > > > > > > > > > > > osc1: clock { > > > > > > compatible = "fixed-clock"; > > > > > > #clock-cells = <0>; > > > > > > clock-frequency = <12000000>; > > > > > > }; > > > > > > > > > > > > rtc: clock { > > > > > > compatible = "fixed-clock"; > > > > > > #clock-cells = <0>; > > > > > > clock-frequency = <32768>; > > > > > > }; > > > > > > > > > > > > mcgout: clock { > > > > > > compatible = "fixed-factor-clock"; > > > > > > #clock-cells = <0>; > > > > > > clocks = <&osc0>; > > > > > > clock-mult = <12>; > > > > > > clock-div = <5>; > > > > > > }; > > > > > > > > > > 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 > > > > > your mcu). Once these "clock providers" are enumerated in DT, then we > > > > > 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 = "fixed-factor-clock"; > > > > > > #clock-cells = <0>; > > > > > > clocks = <&mcgout>; > > > > > > clock-mult = <1>; > > > > > > clock-div = <1>; > > > > > > }; > > > > > > > > > > > > bus: clock { > > > > > > compatible = "fixed-factor-clock"; > > > > > > #clock-cells = <0>; > > > > > > clocks = <&mcgout>; > > > > > > clock-mult = <1>; > > > > > > clock-div = <2>; > > > > > > > > > > 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 = "fsl,kinetis-gate-clock"; > > > > > > reg = <0x40047000 0x1100>; > > > > > > > > > > > > mcg_core_gate: clock-gate { > > > > > > clocks = <&core>; > > > > > > #clock-cells = <2>; > > > > > > }; > > > > > > > > > > > > mcg_bus_gate: clock-gate { > > > > > > clocks = <&bus>; > > > > > > #clock-cells = <2>; > > > > > > }; > > > > > > > > > > > > osc0_erclk_gate: clock-gate { > > > > > > clocks = <&osc0>; > > > > > > #clock-cells = <2>; > > > > > > }; > > > > > > }; > > > > > > > > > > > > uart0: serial@4006a000 { > > > > > > compatible = "fsl,kinetis-lpuart"; > > > > > > reg = <0x4006a000 0x1000>; > > > > > > interrupts = <45>, <46>; > > > > > > interrupt-names = "uart-stat", "uart-err"; > > > > > > clocks = <&mcg_core_gate 3 10>; > > > > > > > > > > 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 = "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 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 > > > > > > > > situation clearer. > > > > > > > > > > > > > > Hi Paul, > > > > > > > > > > > > > > Can you please post the patch in the body of the email instead of an > > > > > > > attachment? It makes it easier to review. Another small nitpick is that > > > > > > > the $SUBJECT for this patch might be better off as something like: > > > > > > > > > > > > > > clk: mcg and sim clock drivers for twr-k70f120m Kinetis SoC > > > > > > > > > > > > > > At least it helps me find the patch I care about when skimming the > > > > > > > series ;-) > > > > > > > > > > > > > > > > > > > > > > > These MCG and SIM registers are used only to determine configuration > > > > > > > > (clock fixed rates and clock signal origins) at run time. > > > > > > > > > > > > > > > > Namely, the real MCGOUTCLK source (in the middle) which is the parent for > > > > > > > > core clock (CCLK) and peripheral clock (PCLK) is determined at run time by > > > > > > > > reading MCG registers, let me quote commit message from Emcraft git repo: > > > > > > > > > > > > > > > > * Determine in run-time what oscillator module (OSC0 or OSC1) is used > > > > > > > > as clock source for the main PLL. > > > > > > > > > > > > > > According to [0] there are three options: a 32k RTC osc clock and osc0 > > > > > > > both feed into a mux. You should model this 32k clock with the > > > > > > > fixed-rate binding. > > > > > > > > > > > > > > > * When OSC1 is selected, assume its frequency to be 12 MHz on all > > > > > > > > boards (there is a 12 MHz oscillator on XTAL1/EXTAL1 on K70-SOM and > > > > > > > > TWR-K70F120M boards). > > > > > > > > > > > > > > > > In my .dts I'm trying to possibly follow real clock hierarchy, but to go > > > > > > > > anywhere behind MCGOUTCLK would require ability to rewrite .dtb e.g. by > > > > > > > > U-boot. But that's too demanding for any potential users of this BSP. So > > > > > > > > let's asume that MCGOUTCLK is the root clock and a parent for CCLK and > > > > > > > > PCLK. > > > > > > > > > > > > > > I'm confused. The point of device tree is to solve problems like this; > > > > > > > i.e. board-specific differences such as different oscillator > > > > > > > frequencies. > > > > > > > > > > > > > > OSC0 and OSC1 should each be a fixed-rate clock in your board-specific > > > > > > > TWR-K70F120M DTS (not a chip-specific file). They do not belong in the > > > > > > > cmu node, and they should use the "fixed-clock" binding. The 32k RTC osc > > > > > > > can probably go in your chip-specific .dtsi as a fixed-rate clock since > > > > > > > it appears to mandated in the reference manual[0]. > > > > > > > > > > > > > > These three fixed-rate clocks are your root clock nodes. Customers only > > > > > > > need to worry about this if they spin a board, and then they will need > > > > > > > to populate the frequencies of OSC0 and OSC1 in their board-specific > > > > > > > .dts. > > > > > > > > > > > > > > Please break clk-kinetis.c into two files: > > > > > > > drivers/clk/kinetis/clk-mcg.c > > > > > > > drivers/clk/kinetis/clk-sim.c > > > > > > > > > > > > > > Below is what your binding/dts should look like: > > > > > > > > > > > > > > { > > > > > > > osc0: clock { > > > > > > > compatible = "fixed-clock"; > > > > > > > #clock-cells = <0>; > > > > > > > clock-frequency = <50000000>; > > > > > > > }; > > > > > > > > > > > > > > osc1: clock { > > > > > > > compatible = "fixed-clock"; > > > > > > > #clock-cells = <0>; > > > > > > > clock-frequency = <12000000>; > > > > > > > }; > > > > > > > > > > > > > > rtc: clock { > > > > > > > compatible = "fixed-clock"; > > > > > > > #clock-cells = <0>; > > > > > > > clock-frequency = <32768>; > > > > > > > }; > > > > > > > > > > > > > > soc: soc { > > > > > > > mcg: clock-controller@40064000 { > > > > > > > compatible = "fsl,kinetis-mcg"; > > > > > > > clock-cells = <1>; > > > > > > > reg = <0x40064000 0x14>; > > > > > > > clocks = <&osc0>, <&osc1>, <&rtc>; > > > > > > > clock-names = "osc0", "osc1", "rtc"; > > > > > > > }; > > > > > > > > > > > > > > sim: clock-controller@40047000 { > > > > > > > compatible = "fsl,kinetis-sim"; > > > > > > > clock-cells = <1>; > > > > > > > reg = <0x40047000 0x1100>; > > > > > > > clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>; > > > > > > > clock-names = "core", "bus", "flexbus", "flash"; > > > > > > > }; > > > > > > > }; > > > > > > > > > > > > > > uart0: serial@4006a000 { > > > > > > > compatible = "fsl,kinetis-lpuart"; > > > > > > > reg = <0x4006a000 0x1000>; > > > > > > > clocks = <&sim SIM_SCGC4_UART1_CLK>; > > > > > > > clock-names = "gate"; > > > > > > > }; > > > > > > > > > > > > > > I removed the interrupts and dma stuff from the uart0 node for clarity. > > > > > > > The above is the only style of binding that I have been accepting for > > > > > > > some time; first declare the clock controller and establish its register > > > > > > > space, and then consumers can consume clocks by providing the phandle to > > > > > > > the controller plus an offset corresponding to a unique clock. The > > > > > > > clock-names property makes it really easy to use with the clkdev stuff > > > > > > > (e.g. clk_get()). > > > > > > > > > > > > > > I've covered this before on the mailing list so here is a link > > > > > > > describing how the qcom bindings do it in detail: > > > > > > > > > > > > > > http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum> > > > > > > > > > > > > > > Technically you could encode the same bits as sub-nodes of the mcg and > > > > > > > sim nodes, but the shared header is how the magic happens with the > > > > > > > driver so it's best to keep the clock controller binding small and > > > > > > > light. > > > > > > > > > > > > > > I think this means you can also get rid of kinetis_of_clk_get_name and > > > > > > > kinetis_clk_gate_get but my brain is tired so I'll leave that as an > > > > > > > exercise to the reader. > > > > > > > > > > > > > > [0] http://cache.freescale.com/files/microcontrollers/doc/ref_manual/K70P256M150SF3RM.pdf > > > > > > > > > > > > > > Regards, > > > > > > > Mike > > > > > > > > > > > > > > > > > > > > > > > In my most recent version I added OSC0ERCLK explicitly as one more root > > > > > > > > clock, since it is also used directly (through CG reg. 1 bit 0) by > > > > > > > > Freescale fec network device whose in-tree driver I'm trying to make > > > > > > > > usable for Kinetis. > > > > > > > > > > > > > > > > On Sat, 4 Jul 2015, Arnd Bergmann wrote: > > > > > > > > > > > > > > > > > On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote: > > > > > > > > >> On Thu, 2 Jul 2015, Paul Osmialowski wrote: > > > > > > > > >>> On Thu, 2 Jul 2015, Arnd Bergmann wrote: > > > > > > > > >>> > > > > > > > > >>>> I wonder if you could move out the fixed rate clocks into their own > > > > > > > > >>>> nodes. Are they actually controlled by the same block? If they are > > > > > > > > >>>> just fixed, you can use the normal binding for fixed rate clocks > > > > > > > > >>>> and only describe the clocks that are related to the driver. > > > > > > > > >>> > > > > > > > > >>> In my view having these clocks grouped together looks more convincing. After > > > > > > > > >>> all, they all share the same I/O regs in order to read configuration. > > > > > > > > >> > > > > > > > > >> The fact that they share a register is not making them a group. That's > > > > > > > > >> just a HW design decision and you need to deal with that by protecting > > > > > > > > >> the register access, but not by trying to group them artificially at > > > > > > > > >> the functional level. > > > > > > > > > > > > > > > > > > I'd disagree with that: The clock controller is the device that owns the > > > > > > > > > registers and that should be one node in DT, as Paul's first version does. > > > > > > > > > > > > > > > > > > The part I'm still struggling with is understanding how the fixed-rate > > > > > > > > > clocks are controlled through those registers. If they are indeed configured > > > > > > > > > through the registers, the name is probably wrong and should be changed > > > > > > > > > to whatever kind of non-fixed clock this is. > > > > > > > > > > > > > > > > > > Arnd > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > > linux-arm-kernel mailing list > > > > > > > > linux-arm-kernel@lists.infradead.org > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > > > > > > > > -- > > > > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > > > > > the body of a message to majordomo@vger.kernel.org > > > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > > > > > -- > > > > 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 > > 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-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From ae43dcd17eec3eb2c3ad4d7cd514295d935655fe Mon Sep 17 00:00:00 2001 From: Paul Osmialowski <pawelo@king.net.pl> Date: Mon, 29 Jun 2015 20:58:55 +0200 Subject: [PATCH 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3. Signed-off-by: Paul Osmialowski <pawelo@king.net.pl> --- .../devicetree/bindings/clock/kinetis-clock.txt | 65 +++ arch/arm/boot/dts/kinetis.dtsi | 29 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-kinetis.c | 475 +++++++++++++++++++++ 4 files changed, 570 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/kinetis-clock.txt create mode 100644 drivers/clk/clk-kinetis.c diff --git a/Documentation/devicetree/bindings/clock/kinetis-clock.txt b/Documentation/devicetree/bindings/clock/kinetis-clock.txt new file mode 100644 index 0000000..e6c1cfa --- /dev/null +++ b/Documentation/devicetree/bindings/clock/kinetis-clock.txt @@ -0,0 +1,65 @@ +* Clock bindings for Freescale Kinetis SoC + +Required properties: +- compatible: Should be "fsl,kinetis-cmu". +- reg: Two address ranges, one for the Multipurpose Clock Genetator (MCG) + register set, one for System Integration Module (SIM) register set. +- Set of clock devices (obligatory): + - fixed-rate-mcgout + Required properties: + - #clock-cells: must be <0>. + - fixed-rate-cclk + Required properties: + - #clock-cells: must be <0>. + - fixed-rate-pclk + Required properties: + - #clock-cells: must be <0>. + - cclk-gate + Required properties: + - #clock-cells: must be <2> (see below). + - pclk-gate + Required properties: + - #clock-cells: must be <2> (see below). + +For clock gate addresses see K70 Sub-Family Reference Manual, Rev. 3 pg. 341 +and later. Notice that addresses are zero-based, so SIM_SCGC1 has address 0, +SIM_SCGC2 has address 1 and so on. The second address component is the bit +index. + +Example: + +cmu@40064000 { + compatible = "fsl,kinetis-cmu"; + reg = <0x40064000 0x14>, <0x40047000 0x1100>; + + mcg_outclk: fixed-rate-mcgout { + #clock-cells = <0>; + }; + + mcg_cclk: fixed-rate-cclk { + #clock-cells = <0>; + }; + + mcg_pclk: fixed-rate-pclk { + #clock-cells = <0>; + }; + + mcg_cclk_gate: cclk-gate { + #clock-cells = <2>; + }; + + mcg_pclk_gate: pclk-gate { + #clock-cells = <2>; + }; +}; + +uart1: serial@4006b000 { + compatible = "fsl,kinetis-lpuart"; + reg = <0x4006b000 0x1000>; + interrupts = <47>, <48>; + interrupt-names = "uart-stat", "uart-err"; + clocks = <&mcg_cclk_gate 3 11>; + clock-names = "ipg"; + dmas = <&edma 0 4>; + dma-names = "rx"; +}; diff --git a/arch/arm/boot/dts/kinetis.dtsi b/arch/arm/boot/dts/kinetis.dtsi index 93d2a8a..42a11c7 100644 --- a/arch/arm/boot/dts/kinetis.dtsi +++ b/arch/arm/boot/dts/kinetis.dtsi @@ -3,3 +3,32 @@ * */ #include "armv7-m.dtsi" + +/ { + soc { + cmu@40064000 { + compatible = "fsl,kinetis-cmu"; + reg = <0x40064000 0x14>, <0x40047000 0x1100>; + + mcg_outclk: fixed-rate-mcgout { + #clock-cells = <0>; + }; + + mcg_cclk: fixed-rate-cclk { + #clock-cells = <0>; + }; + + mcg_pclk: fixed-rate-pclk { + #clock-cells = <0>; + }; + + mcg_cclk_gate: cclk-gate { + #clock-cells = <2>; + }; + + mcg_pclk_gate: pclk-gate { + #clock-cells = <2>; + }; + }; + }; +}; diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 63418cf..412d76b 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -24,6 +24,7 @@ obj-$(CONFIG_COMMON_CLK_CDCE706) += clk-cdce706.o obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o obj-$(CONFIG_ARCH_EFM32) += clk-efm32gg.o obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o +obj-$(CONFIG_ARCH_KINETIS) += clk-kinetis.o obj-$(CONFIG_MACH_LOONGSON32) += clk-ls1x.o obj-$(CONFIG_COMMON_CLK_MAX_GEN) += clk-max-gen.o obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o diff --git a/drivers/clk/clk-kinetis.c b/drivers/clk/clk-kinetis.c new file mode 100644 index 0000000..a6e8a28 --- /dev/null +++ b/drivers/clk/clk-kinetis.c @@ -0,0 +1,475 @@ +/* + * clk-kinetis.c - Clock driver for Kinetis K70 MCG + * + * Based on legacy pre-OF code by Alexander Potashev <aspotashev@emcraft.com> + * + * Copyright (C) 2015 Paul Osmialowski <pawelo@king.net.pl> + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation. + */ + +#include <linux/io.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/clk-provider.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_device.h> + +enum kinetis_clk_ids { + KINETIS_CLK_MCGOUT = 0, + KINETIS_CLK_CCLK, + KINETIS_CLK_PCLK, + KINETIS_CLK_CCLK_GATE, + KINETIS_CLK_PCLK_GATE, + KINETIS_CLK_NUM +}; + +static const struct { + enum kinetis_clk_ids id; + const char *name; +} kinetis_clks[KINETIS_CLK_NUM] = { + { .id = KINETIS_CLK_MCGOUT, .name = "fixed-rate-mcgout", }, + { .id = KINETIS_CLK_CCLK, .name = "fixed-rate-cclk", }, + { .id = KINETIS_CLK_PCLK, .name = "fixed-rate-pclk", }, + { .id = KINETIS_CLK_CCLK_GATE, .name = "cclk-gate", }, + { .id = KINETIS_CLK_PCLK_GATE, .name = "pclk-gate", }, +}; + +/* + * Frequencies on OSC0 (EXTAL0/XTAL0) and OSC1 (EXTAL1/XTAL1) + * + * These frequencies should be set to the same values as in U-Boot. + */ +#define KINETIS_OSC0_RATE 50000000 /* 50 MHz */ +#define KINETIS_OSC1_RATE 12000000 /* 12 MHz */ + +#define KINETIS_SIM_CG_NUMREGS 7 + +/* + * System Integration Module (SIM) register map + * + * This map actually covers two hardware modules: + * 1. SIM low-power logic, at 0x40047000 + * 2. System integration module (SIM), at 0x40048000 + */ +struct kinetis_sim_regs { + u32 sopt1; /* System Options Register 1 */ + u32 rsv0[1024]; + u32 sopt2; /* System Options Register 2 */ + u32 rsv1; + u32 sopt4; /* System Options Register 4 */ + u32 sopt5; /* System Options Register 5 */ + u32 sopt6; /* System Options Register 6 */ + u32 sopt7; /* System Options Register 7 */ + u32 rsv2[2]; + u32 sdid; /* System Device Identification Register */ + u32 scgc[KINETIS_SIM_CG_NUMREGS]; /* Clock Gating Regs 1...7 */ + u32 clkdiv1; /* System Clock Divider Register 1 */ + u32 clkdiv2; /* System Clock Divider Register 2 */ + u32 fcfg1; /* Flash Configuration Register 1 */ + u32 fcfg2; /* Flash Configuration Register 2 */ + u32 uidh; /* Unique Identification Register High */ + u32 uidmh; /* Unique Identification Register Mid-High */ + u32 uidml; /* Unique Identification Register Mid Low */ + u32 uidl; /* Unique Identification Register Low */ + u32 clkdiv3; /* System Clock Divider Register 3 */ + u32 clkdiv4; /* System Clock Divider Register 4 */ + u32 mcr; /* Misc Control Register */ +}; +#define KINETIS_SIM_PTR(base, reg) \ + (&(((struct kinetis_sim_regs *)(base))->reg)) + +/* + * System Clock Divider Register 1 + */ +/* Clock 1 output divider value (for the core/system clock) */ +#define KINETIS_SIM_CLKDIV1_OUTDIV1_BITS 28 +#define KINETIS_SIM_CLKDIV1_OUTDIV1_MSK \ + (((1 << 4) - 1) << KINETIS_SIM_CLKDIV1_OUTDIV1_BITS) +/* Clock 2 output divider value (for the peripheral clock) */ +#define KINETIS_SIM_CLKDIV1_OUTDIV2_BITS 24 +#define KINETIS_SIM_CLKDIV1_OUTDIV2_MSK \ + (((1 << 4) - 1) << KINETIS_SIM_CLKDIV1_OUTDIV2_BITS) + +/* + * System Clock Divider Register 2 + */ +/* USB HS clock divider fraction */ +#define KINETIS_SIM_CLKDIV2_USBHSFRAC_BIT 8 +#define KINETIS_SIM_CLKDIV2_USBHSFRAC_MSK \ + (1 << KINETIS_SIM_CLKDIV2_USBHSFRAC_BIT) +/* USB HS clock divider divisor */ +#define KINETIS_SIM_CLKDIV2_USBHSDIV_BIT 9 +#define KINETIS_SIM_CLKDIV2_USBHSDIV_MSK \ + (7 << KINETIS_SIM_CLKDIV2_USBHSDIV_BIT) + +/* + * MCG Control 5 Register + */ +/* PLL External Reference Divider */ +#define KINETIS_MCG_C5_PRDIV_BITS 0 +#define KINETIS_MCG_C5_PRDIV_MSK \ + (((1 << 3) - 1) << KINETIS_MCG_C5_PRDIV_BITS) +/* PLL Stop Enable */ +#define KINETIS_MCG_C5_PLLSTEN_MSK (1 << 5) +/* PLL Clock Enable */ +#define KINETIS_MCG_C5_PLLCLKEN_MSK (1 << 6) +/* PLL External Reference Select (for K70@120MHz) */ +#define KINETIS_MCG_C5_PLLREFSEL_BIT 7 +#define KINETIS_MCG_C5_PLLREFSEL_MSK (1 << KINETIS_MCG_C5_PLLREFSEL_BIT) +/* + * MCG Control 6 Register + */ +/* VCO Divider */ +#define KINETIS_MCG_C6_VDIV_BITS 0 +#define KINETIS_MCG_C6_VDIV_MSK \ + (((1 << 5) - 1) << KINETIS_MCG_C6_VDIV_BITS) +/* PLL Select */ +#define KINETIS_MCG_C6_PLLS_MSK (1 << 6) +/* + * MCG Control 11 Register + */ +/* PLL1 External Reference Divider */ +#define KINETIS_MCG_C11_PRDIV_BITS 0 +#define KINETIS_MCG_C11_PRDIV_MSK \ + (((1 << 3) - 1) << KINETIS_MCG_C11_PRDIV_BITS) +/* PLL Clock Select: PLL0 or PLL1 */ +#define KINETIS_MCG_C11_PLLCS_MSK (1 << 4) +/* PLL1 Stop Enable */ +#define KINETIS_MCG_C11_PLLSTEN1_MSK (1 << 5) +/* PLL1 Clock Enable */ +#define KINETIS_MCG_C11_PLLCLKEN1_MSK (1 << 6) +/* PLL1 External Reference Select (for K70@120MHz) */ +#define KINETIS_MCG_C11_PLLREFSEL1_BIT 7 +#define KINETIS_MCG_C11_PLLREFSEL1_MSK (1 << KINETIS_MCG_C11_PLLREFSEL1_BIT) +/* + * MCG Control 12 Register + */ +/* VCO1 Divider */ +#define KINETIS_MCG_C12_VDIV1_BITS 0 +#define KINETIS_MCG_C12_VDIV1_MSK \ + (((1 << 5) - 1) << KINETIS_MCG_C12_VDIV1_BITS) + +/* + * Multipurpose Clock Generator (MCG) register map + * + * See Chapter 25 of the K70 Reference Manual + */ +struct kinetis_mcg_regs { + u8 c1; /* MCG Control 1 Register */ + u8 c2; /* MCG Control 2 Register */ + u8 c3; /* MCG Control 3 Register */ + u8 c4; /* MCG Control 4 Register */ + u8 c5; /* MCG Control 5 Register */ + u8 c6; /* MCG Control 6 Register */ + u8 status; /* MCG Status Register */ + u8 rsv0; + u8 atc; /* MCG Auto Trim Control Register */ + u8 rsv1; + u8 atcvh; /* MCG Auto Trim Compare Value High Register */ + u8 atcvl; /* MCG Auto Trim Compare Value Low Register */ + u8 c7; /* MCG Control 7 Register */ + u8 c8; /* MCG Control 8 Register */ + u8 rsv2; + u8 c10; /* MCG Control 10 Register */ + u8 c11; /* MCG Control 11 Register */ + u8 c12; /* MCG Control 12 Register */ + u8 status2; /* MCG Status 2 Register */ + u8 rsv3; +}; +#define KINETIS_MCG_PTR(base, reg) \ + (&(((struct kinetis_mcg_regs *)(base))->reg)) + +struct kinetis_clk_gate { + const char *clk_gate_name; + struct clk *clk; + u32 reg; + u32 idx; + struct list_head clk_list; +}; + +struct kinetis_clk_gate_data { + const char *clk_parent_name; + void __iomem *sim; + struct list_head clk_gate_list; +}; + +static struct clk *kinetis_find_clk_gate( + struct kinetis_clk_gate_data *clk_gate_data, u32 reg, u32 idx) +{ + struct kinetis_clk_gate *gate; + + list_for_each_entry(gate, &clk_gate_data->clk_gate_list, clk_list) + if ((gate->reg == reg) && (gate->idx == idx)) + return gate->clk; + + return NULL; +} + +static struct clk *kinetis_clk_gate_get(struct of_phandle_args *clkspec, + void *data) +{ + struct kinetis_clk_gate_data *clk_gate_data = data; + struct kinetis_clk_gate *gate; + u32 reg = clkspec->args[0]; + u32 idx = clkspec->args[1]; + struct clk *clk; + + clk = kinetis_find_clk_gate(clk_gate_data, reg, idx); + if (clk) + return clk; + + gate = kzalloc(sizeof(struct kinetis_clk_gate), GFP_KERNEL); + if (!gate) + return ERR_PTR(-ENOMEM); + gate->clk_gate_name = kasprintf(GFP_KERNEL, "%s:%u:%u", + clkspec->np->full_name, reg, idx); + + clk = clk_register_gate(NULL, gate->clk_gate_name, + clk_gate_data->clk_parent_name, 0, + KINETIS_SIM_PTR(clk_gate_data->sim, scgc[reg]), + idx, 0, NULL); + if (IS_ERR(clk)) { + pr_err("Cannot register gate to clock %s\n", + clk_gate_data->clk_parent_name); + kfree_const(gate->clk_gate_name); + kfree(gate); + return clk; + } + + gate->clk = clk; + gate->reg = reg; + gate->idx = idx; + + list_add(&gate->clk_list, &clk_gate_data->clk_gate_list); + + return clk; +} + +static int kinetis_of_register_fixed_rate_root(struct device_node *np, + u32 clock_val) +{ + struct clk *clk; + int ret; + + clk = clk_register_fixed_rate(NULL, np->full_name, NULL, CLK_IS_ROOT, + clock_val); + if (IS_ERR(clk)) { + pr_err("Could not register clock %s\n", np->full_name); + return PTR_ERR(clk); + } + + ret = of_clk_add_provider(np, of_clk_src_simple_get, clk); + if (ret < 0) { + pr_err("Could not add clock provider %s\n", np->full_name); + clk_unregister(clk); + return ret; + } + + return 0; +} + +static int kinetis_of_register_fixed_rate(struct device_node *np, + struct device_node *parent_clk, + u32 clock_val) +{ + struct clk *clk; + int ret; + + clk = clk_register_fixed_rate(NULL, np->full_name, + parent_clk->full_name, 0, clock_val); + if (IS_ERR(clk)) { + pr_err("Could not register clock %s\n", np->full_name); + return PTR_ERR(clk); + } + + ret = of_clk_add_provider(np, of_clk_src_simple_get, clk); + if (ret < 0) { + pr_err("Could not add clock provider %s\n", np->full_name); + clk_unregister(clk); + return ret; + } + + return 0; +} + +static int kinetis_of_register_clk_gate(struct device_node *np, + struct device_node *parent_clk, + void __iomem *sim) +{ + struct kinetis_clk_gate_data *clk_gate; + int ret; + + clk_gate = kzalloc(sizeof(struct kinetis_clk_gate_data), GFP_KERNEL); + if (!clk_gate) + return -ENOMEM; + + clk_gate->clk_parent_name = parent_clk->full_name; + clk_gate->sim = sim; + INIT_LIST_HEAD(&clk_gate->clk_gate_list); + + ret = of_clk_add_provider(np, kinetis_clk_gate_get, clk_gate); + if (ret < 0) { + pr_err("Could not add clock provider %s\n", np->full_name); + kfree(clk_gate); + return ret; + } + + return 0; +} + +static void __init kinetis_mcg_init(struct device_node *np) +{ + const int vco_div = 2; + const int vdiv_min = 16; + u32 clock_val_mcgout; + u32 clock_val_cclk; + u32 clock_val_pclk; + void __iomem *base; + void __iomem *sim; + int pll_sel; + int osc_sel; + unsigned long mcgout; + struct device_node *child; + struct device_node *clk_nodes[KINETIS_CLK_NUM]; + int i; + + for (i = 0; i < KINETIS_CLK_NUM; i++) + clk_nodes[i] = NULL; + + base = of_iomap(np, 0); + if (!base) { + pr_err("Failed to map address range for kinetis,mcg node\n"); + return; + } + + sim = of_iomap(np, 1); + if (!sim) { + pr_err("Failed to map address range for kinetis SIM module\n"); + iounmap(base); + return; + } + + /* + * Check whether PLL0 or PLL1 is used for MCGOUTCLK + */ + pll_sel = !!(ioread8(KINETIS_MCG_PTR(base, c11)) & + KINETIS_MCG_C11_PLLCS_MSK); + + /* + * Check whether OSC0 or OSC1 is used to source the main PLL + */ + if (pll_sel) + osc_sel = !!(ioread8(KINETIS_MCG_PTR(base, c11)) & + KINETIS_MCG_C11_PLLREFSEL1_MSK); + else + osc_sel = !!(ioread8(KINETIS_MCG_PTR(base, c5)) & + KINETIS_MCG_C5_PLLREFSEL_MSK); + + /* + * Start with the MCG input clock + */ + mcgout = osc_sel ? KINETIS_OSC1_RATE : KINETIS_OSC0_RATE; + + /* + * Apply dividers and multipliers of the selected PLL + */ + if (pll_sel) { + /* + * PLL1 internal divider (PRDIV) + */ + mcgout /= ((ioread8(KINETIS_MCG_PTR(base, c11)) & + KINETIS_MCG_C11_PRDIV_MSK) >> KINETIS_MCG_C11_PRDIV_BITS) + 1; + + /* + * PLL1 multiplication factor (VDIV) + */ + mcgout *= ((ioread8(KINETIS_MCG_PTR(base, c12)) & + KINETIS_MCG_C12_VDIV1_MSK) >> KINETIS_MCG_C12_VDIV1_BITS) + + vdiv_min; + } else { + /* + * PLL0 internal divider (PRDIV) + */ + mcgout /= ((ioread8(KINETIS_MCG_PTR(base, c5)) & + KINETIS_MCG_C5_PRDIV_MSK) >> + KINETIS_MCG_C5_PRDIV_BITS) + 1; + + /* + * PLL0 multiplication factor (VDIV) + */ + mcgout *= ((ioread8(KINETIS_MCG_PTR(base, c6)) & + KINETIS_MCG_C6_VDIV_MSK) >> + KINETIS_MCG_C6_VDIV_BITS) + vdiv_min; + } + + /* + * Apply the PLL output divider + */ + mcgout /= vco_div; + + clock_val_mcgout = mcgout; + + clock_val_cclk = mcgout / + (((ioread32(KINETIS_SIM_PTR(sim, clkdiv1)) & + KINETIS_SIM_CLKDIV1_OUTDIV1_MSK) >> + KINETIS_SIM_CLKDIV1_OUTDIV1_BITS) + 1); + + /* + * Peripheral (bus) clock + */ + clock_val_pclk = mcgout / + (((ioread32(KINETIS_SIM_PTR(sim, clkdiv1)) & + KINETIS_SIM_CLKDIV1_OUTDIV2_MSK) >> + KINETIS_SIM_CLKDIV1_OUTDIV2_BITS) + 1); + + for_each_child_of_node(np, child) { + if (!of_device_is_available(child)) + continue; + + for (i = 0; i < KINETIS_CLK_NUM; i++) { + if (!of_node_cmp(child->name, kinetis_clks[i].name)) { + if (clk_nodes[kinetis_clks[i].id]) { + pr_err("more than one %s specified\n", + child->name); + goto fail; + } else + clk_nodes[kinetis_clks[i].id] = child; + } + } + } + + for (i = 0; i < KINETIS_CLK_NUM; i++) { + if (!(clk_nodes[kinetis_clks[i].id])) { + pr_err("One of obligatory clocks NOT specified\n"); + goto fail; + } + } + + if (kinetis_of_register_fixed_rate_root(clk_nodes[KINETIS_CLK_MCGOUT], + clock_val_mcgout)) + goto fail; + + if (!(kinetis_of_register_fixed_rate(clk_nodes[KINETIS_CLK_CCLK], + clk_nodes[KINETIS_CLK_MCGOUT], clock_val_cclk))) + kinetis_of_register_clk_gate(clk_nodes[KINETIS_CLK_CCLK_GATE], + clk_nodes[KINETIS_CLK_CCLK], sim); + + if (!(kinetis_of_register_fixed_rate(clk_nodes[KINETIS_CLK_PCLK], + clk_nodes[KINETIS_CLK_MCGOUT], clock_val_pclk))) + kinetis_of_register_clk_gate(clk_nodes[KINETIS_CLK_PCLK_GATE], + clk_nodes[KINETIS_CLK_PCLK], sim); + + return; + +fail: + + iounmap(sim); + iounmap(base); +} + +CLK_OF_DECLARE(kinetis_mcg, "fsl,kinetis-cmu", kinetis_mcg_init); -- 2.3.6