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 at 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 at 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)

quoted

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 ;-)

quoted

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.

quoted

      * 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 at 40064000 {
			compatible = "fsl,kinetis-mcg";
			clock-cells = <1>;
			reg = <0x40064000 0x14>;
			clocks = <&osc0>, <&osc1>, <&rtc>;
			clock-names = "osc0", "osc1", "rtc";
		};

		sim: clock-controller at 40047000 {
			compatible = "fsl,kinetis-sim";
			clock-cells = <1>;
			reg = <0x40047000 0x1100>;
			clocks = <&mcg MCG_MCGOUTCLK_DIV1>, <&mcg MCG_MCGOUTCLK_DIV2>, <&mcg MCG_MCGOUTCLK_DIV3> <&mcg MCG_MCGOUTCLK_DIV4>;
			clock-names = "core", "bus", "flexbus", "flash";
		};
	};

	uart0: serial at 4006a000 {
		compatible = "fsl,kinetis-lpuart";
		reg = <0x4006a000 0x1000>;
		clocks = <&sim SIM_SCGC4_UART1_CLK>;
		clock-names = "gate";
	};

I removed the interrupts and dma stuff from the uart0 node for clarity.
The above is the only style of binding that I have been accepting for
some time; first declare the clock controller and establish its register
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

quoted

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:

quoted

On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

quoted

On Thu, 2 Jul 2015, Paul Osmialowski wrote:

quoted

On Thu, 2 Jul 2015, Arnd Bergmann wrote:

quoted

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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel