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@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
