Add initial support for Freescale Kinetis TWR-K70F120M development kit

Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.

Signed-off-by Paul Osmialowski <[email protected]>

---
 .../devicetree/bindings/clock/kinetis-clock.txt    |  63 +++
 arch/arm/boot/dts/kinetis.dtsi                     |  36 ++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-kinetis.c                          | 463 +++++++++++++++++++++
 4 files changed, 563 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/kinetis-clock.txt
 create mode 100644 drivers/clk/clk-kinetis.c

Documentation/devicetree/bindings/clock/kinetis-clock.txt new file +2 -0

new file mode 100644
index 0000000..63af6a5
@@ -0,0 +1,63 @@
+* Clock bindings for Freescale Kinetis SoC
+

Quoted 20 lines · On Tuesday 30 June 2015 14:27:24 Paul Osmialowski wrote:

Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.

Signed-off-by Paul Osmialowski <[email protected]>

---
 .../devicetree/bindings/clock/kinetis-clock.txt    |  63 +++
 arch/arm/boot/dts/kinetis.dtsi                     |  36 ++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-kinetis.c                          | 463 +++++++++++++++++++++
 4 files changed, 563 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/kinetis-clock.txt
 create mode 100644 drivers/clk/clk-kinetis.c

Documentation/devicetree/bindings/clock/kinetis-clock.txt new file +2 -0

new file mode 100644
index 0000000..63af6a5
@@ -0,0 +1,63 @@
+* Clock bindings for Freescale Kinetis SoC
+

Documentation/devicetree/bindings/clock/kinetis-clock.txt line 12

Quoted 10 lines

Documentation/devicetree/bindings/clock/kinetis-clock.txt new file +10 -0

+Required properties:
+- compatible: Should be "fsl,kinetis-cmu".
+- reg: Two address ranges, one for the Clock Genetator register set,
+	one for System Integration Module register set.
+- Set of clock devices: one fixed-rate-root, fixed-rate clocks and clock-gates.
+
+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.

Please document the sub-nodes that are allowed, and the format of the clock specifiers.

Documentation/devicetree/bindings/clock/kinetis-clock.txt line 25

Quoted 13 lines

Documentation/devicetree/bindings/clock/kinetis-clock.txt new file +13 -0

+
+Example:
+
+cmu at 40064000 {
+	compatible = "fsl,kinetis-cmu";
+	reg = <0x40064000 0x14>, <0x40047000 0x1100>;
+
+	mcg_outclk: fixed-rate-root at mcgout {
+		device_type = "mcgout";
+		#clock-cells = <0>;
+	};
+
+	mcg_cclk: fixed-rate at cclk {

'@' is a reserved character here that is used before the address of the device, so this has to be a hexadecimal number without leading '0x', and it should match the 'reg' property of the device.

Documentation/devicetree/bindings/clock/kinetis-clock.txt line 29

Quoted 4 lines

Documentation/devicetree/bindings/clock/kinetis-clock.txt new file +4 -0

+		device_type = "cclk";
+		#clock-cells = <0>;
+		clocks = <&mcg_outclk>;
+	};

The device_type property here is not a standard identifier, and you don't list it as an optional or mandatory property.

Please remove it and instead use the compatible property, the name or the address.

Arnd

Hi Arnd,

Can you look at attached candidate for the third iteration? Is it any better now?

Thanks,
Paul

Quoted 73 lines · Arnd Bergmann

Quoted 26 lines · On Tuesday 30 June 2015 14:27:24 Paul Osmialowski wrote:

Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.

Signed-off-by Paul Osmialowski <[email protected]>

---

  .../devicetree/bindings/clock/kinetis-clock.txt    |  63 +++
  arch/arm/boot/dts/kinetis.dtsi                     |  36 ++
  drivers/clk/Makefile                               |   1 +
  drivers/clk/clk-kinetis.c                          | 463 
  +++++++++++++++++++++
  4 files changed, 563 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..63af6a5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/kinetis-clock.txt @@ -0,0 +1,63 @@
+* Clock bindings for Freescale Kinetis SoC
+
+Required properties:

 +- compatible: Should be "fsl,kinetis-cmu".
 +- reg: Two address ranges, one for the Clock Genetator register set,

Quoted 11 lines

+ one for System Integration Module register set.
+- Set of clock devices: one fixed-rate-root, fixed-rate clocks and clock-gates.
+
+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.

Please document the sub-nodes that are allowed, and the format of the clock specifiers.

Quoted 5 lines

+
+Example:
+
+cmu at 40064000 {
+ compatible = "fsl,kinetis-cmu";

Quoted 8 lines

+ reg = <0x40064000 0x14>, <0x40047000 0x1100>;
+
+ mcg_outclk: fixed-rate-root at mcgout {
+ device_type = "mcgout";
+ #clock-cells = <0>;
+ };
+
+ mcg_cclk: fixed-rate at cclk {

'@' is a reserved character here that is used before the address of the device, so this has to be a hexadecimal number without leading '0x', and it should match the 'reg' property of the device.

Quoted 4 lines

+ device_type = "cclk";
+ #clock-cells = <0>;
+ clocks = <&mcg_outclk>;
+ };

The device_type property here is not a standard identifier, and you don't list it as an optional or mandatory property.

Please remove it and instead use the compatible property, the name or the address.

Arnd

Nah, I've found this code hard to maintain. I'm attaching simplified version.

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

Quoted 4 lines · Arnd Bergmann

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.

Quoted 9 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 2 lines

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.

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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

Hi Arnd,

I'm attaching excerpt from Kinetis reference manual that may make situation clearer.

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.
* 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.

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.

Quoted 29 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 8 lines

Quoted 3 lines

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

Hi Arnd,

I'm attaching excerpt from Kinetis reference manual that may make situation clearer.

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.

* 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.

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

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

Quoted 125 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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.

Quoted 50 lines

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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoting Paul Osmialowski (2015-07-26 13:24:08)

Quoted 4 lines

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.

Quoted 23 lines

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

Quoted 7 lines

        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.

Quoted 15 lines

        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.

Quoted 21 lines

        };

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

Quoted 8 lines

                };

                uart0: serial at 4006a000 {
                        compatible = "fsl,kinetis-lpuart";
                        reg = <0x4006a000 0x1000>;
                        interrupts = <45>, <46>;
                        interrupt-names = "uart-stat", "uart-err";
                        clocks = <&mcg_core_gate 3 10>;

Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example.

Quoted 9 lines

                        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.

Quoted 4 lines

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

Quoted 186 lines

Thanks,
Paul

Quoted 125 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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.

Quoted 50 lines

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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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>.

From my understanding the DT folks do not like register-level or

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

Quoted 168 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-26 13:24:08)

Quoted 4 lines

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.

Quoted 23 lines

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

Quoted 7 lines

        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.

Quoted 15 lines

        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.

Quoted 21 lines

        };

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

Quoted 8 lines

                };

                uart0: serial at 4006a000 {
                        compatible = "fsl,kinetis-lpuart";
                        reg = <0x4006a000 0x1000>;
                        interrupts = <45>, <46>;
                        interrupt-names = "uart-stat", "uart-err";
                        clocks = <&mcg_core_gate 3 10>;

Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example.

Quoted 9 lines

                        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.

Quoted 4 lines

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.

Quoted 194 lines

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

Quoted 186 lines

Thanks,
Paul

Quoted 125 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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.

Quoted 50 lines

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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoting Paul Osmialowski (2015-07-28 13:30:17)

Quoted 13 lines

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.:

Quoted 7 lines

#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?

Quoted 5 lines

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,

Quoted 1 line

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

Quoted 11 lines

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

Quoted 373 lines

Thanks,
Paul

Quoted 168 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-26 13:24:08)

Quoted 4 lines

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.

Quoted 23 lines

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

Quoted 7 lines

        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.

Quoted 15 lines

        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.

Quoted 21 lines

        };

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

Quoted 8 lines

                };

                uart0: serial at 4006a000 {
                        compatible = "fsl,kinetis-lpuart";
                        reg = <0x4006a000 0x1000>;
                        interrupts = <45>, <46>;
                        interrupt-names = "uart-stat", "uart-err";
                        clocks = <&mcg_core_gate 3 10>;

Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example.

Quoted 9 lines

                        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.

Quoted 4 lines

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.

Quoted 194 lines

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

Quoted 186 lines

Thanks,
Paul

Quoted 125 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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.

Quoted 50 lines

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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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 at 40064000 {
                        compatible = "fsl,kinetis-mcg";
                        clock-cells = <0>;

                        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>,
                                 <&osc0>;

                        clock-names = "mcgoutclk", "enet";
                };

                uart0: serial at 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.

Quoted 58 lines

Quoting Paul Osmialowski (2015-07-28 13:30:17)

Quoted 13 lines

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.:

Quoted 7 lines

#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?

Quoted 5 lines

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,

Quoted 1 line

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

Quoted 11 lines

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.

Quoted 382 lines

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

Quoted 373 lines

Thanks,
Paul

Quoted 168 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-26 13:24:08)

Quoted 4 lines

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.

Quoted 23 lines

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

Quoted 7 lines

        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.

Quoted 15 lines

        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.

Quoted 21 lines

        };

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

Quoted 8 lines

                };

                uart0: serial at 4006a000 {
                        compatible = "fsl,kinetis-lpuart";
                        reg = <0x4006a000 0x1000>;
                        interrupts = <45>, <46>;
                        interrupt-names = "uart-stat", "uart-err";
                        clocks = <&mcg_core_gate 3 10>;

Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example.

Quoted 9 lines

                        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.

Quoted 4 lines

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.

Quoted 194 lines

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

Quoted 186 lines

Thanks,
Paul

Quoted 125 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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.

Quoted 50 lines

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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoting Paul Osmialowski (2015-07-30 14:40:48)

Quoted 6 lines

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.

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.

Quoted 3 lines

                        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?

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>.

Quoted 5 lines

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.

Besides my annoying questions above this binding is starting to shape up very well. Thanks for your patience!

Regards,
Mike

Quoted 447 lines · Michael Turquette

Quoted 58 lines

Quoting Paul Osmialowski (2015-07-28 13:30:17)

Quoted 13 lines

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.:

Quoted 7 lines

#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?

Quoted 5 lines

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,

Quoted 1 line

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

Quoted 11 lines

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.

Quoted 382 lines

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

Quoted 373 lines

Thanks,
Paul

Quoted 168 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-26 13:24:08)

Quoted 4 lines

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.

Quoted 23 lines

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

Quoted 7 lines

        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.

Quoted 15 lines

        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.

Quoted 21 lines

        };

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

Quoted 8 lines

                };

                uart0: serial at 4006a000 {
                        compatible = "fsl,kinetis-lpuart";
                        reg = <0x4006a000 0x1000>;
                        interrupts = <45>, <46>;
                        interrupt-names = "uart-stat", "uart-err";
                        clocks = <&mcg_core_gate 3 10>;

Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example.

Quoted 9 lines

                        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.

Quoted 4 lines

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.

Quoted 194 lines

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

Quoted 186 lines

Thanks,
Paul

Quoted 125 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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.

Quoted 50 lines

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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi Mike,

Quoted 18 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-30 14:40:48)

Quoted 6 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 5 lines

                        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.

Quoted 18 lines

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...

Quoted 4 lines

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.

Quoted 11 lines

Quoted 5 lines

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

Quoted 455 lines

Besides my annoying questions above this binding is starting to shape up very well. Thanks for your patience!

Regards,
Mike

Quoted 447 lines · Michael Turquette

Quoted 58 lines

Quoting Paul Osmialowski (2015-07-28 13:30:17)

Quoted 13 lines

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.:

Quoted 7 lines

#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?

Quoted 5 lines

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,

Quoted 1 line

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

Quoted 11 lines

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.

Quoted 382 lines

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

Quoted 373 lines

Thanks,
Paul

Quoted 168 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-26 13:24:08)

Quoted 4 lines

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.

Quoted 23 lines

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

Quoted 7 lines

        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.

Quoted 15 lines

        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.

Quoted 21 lines

        };

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

Quoted 8 lines

                };

                uart0: serial at 4006a000 {
                        compatible = "fsl,kinetis-lpuart";
                        reg = <0x4006a000 0x1000>;
                        interrupts = <45>, <46>;
                        interrupt-names = "uart-stat", "uart-err";
                        clocks = <&mcg_core_gate 3 10>;

Magic numbers are not good. dtc has been able to use preprocessor macros for a while now which means we can use constants instead of magic numbers. Please look at the shared header in the qcom binding for an example.

Quoted 9 lines

                        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.

Quoted 4 lines

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.

Quoted 194 lines

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

Quoted 186 lines

Thanks,
Paul

Quoted 125 lines · Michael Turquette

Quoting Paul Osmialowski (2015-07-04 14:50:03)

Quoted 4 lines

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 10 lines

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 9 lines

* 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.

Quoted 50 lines

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 40 lines

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.

Quoted 20 lines · Arnd Bergmann

Quoted 8 lines · On Friday 03 July 2015 00:08:27 Thomas Gleixner wrote:

Quoted 7 lines · Paul Osmialowski

Quoted 4 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.

Signed-off-by Paul Osmialowski <[email protected]>

drivers/clk/clk-kinetis.c new file +14 -0

+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 */

Quoted 3 lines · Paul Osmialowski

Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.

Signed-off-by Paul Osmialowski <[email protected]>

(...)

Quoted 14 lines

drivers/clk/clk-kinetis.c new file +14 -0

+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 */

drivers/clk/clk-kinetis.c line 62

Quoted 10 lines

drivers/clk/clk-kinetis.c new file +10 -0

+       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 */
+};

Now there is this design pattern where you copy the datasheet register map to a struct again.

This is not good if there is a second revision of the hardware and some registers are shuffled around. IMO it is better to just use #defines for register
offsets, so you can do exceptions later. Else a new hardware revision leads to a new struct with new accessor functions etc etc.

Hi Linus,

Thanks for all of your comments, I'll consider them during my works on the next iteration. However, I have doubts about this one:

Quoted 34 lines · Linus Walleij

Quoted 3 lines · Paul Osmialowski

Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.

Signed-off-by Paul Osmialowski <[email protected]>

(...)

Quoted 14 lines

drivers/clk/clk-kinetis.c new file +14 -0

+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 */

drivers/clk/clk-kinetis.c line 62

Quoted 10 lines

drivers/clk/clk-kinetis.c new file +10 -0

+       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 */
+};

Now there is this design pattern where you copy the datasheet register map to a struct again.

Quoted 6 lines

This is not good if there is a second revision of the hardware and some registers are shuffled around. IMO it is better to just use #defines for register
offsets, so you can do exceptions later. Else a new hardware revision leads to a new struct with new accessor functions etc etc.

I don't see how replacing this structure with bunch of defines could make anyones life easier. As registers are shuffled around due to updated hardware revision they could be shuffled in this structure too. Doing this with buch of defines would require eager and careful adaptation of all the defines. I don't see how this could be easier.

Note that I'm not making any instances of the structure (it is used only for casting), so shuffling its fields around should not affect the code that follows.

After recent purge it is only used within this macro:

#define KINETIS_SIM_PTR(base, reg) \
        (&(((struct kinetis_sim_regs *)(base))->reg))

...and used like this:

        ioread32(KINETIS_SIM_PTR(sim, clkdiv1));

IMHO changing the struct internals does not require subsequent changes in any of those.

Thanks,
Paul

Documentation/devicetree/bindings/timer/fsl,kinetis-pit-timer.txt new file +6 -0

+Example:
+
+aliases {
+	pit0 = &pit0;
+	pit1 = &pit1;
+	pit2 = &pit2;

/ {

arch/arm/boot/dts/kinetis.dtsi +10 -0

+	aliases {
+		pit0 = &pit0;
+		pit1 = &pit1;
+		pit2 = &pit2;
+		pit3 = &pit3;
+	};
+
 	soc {
+		pit at 40037000 {
+			compatible = "fsl,kinetis-pit-timer";
+			reg = <0x40037000 0x100>;

drivers/clocksource/timer-kinetis.c new file +7 -0

+#define KINETIS_PIT_PTR(base, reg) \
+	(&(((struct kinetis_pit_channel_regs *)(base))->reg))
+#define KINETIS_PIT_RD(be, base, reg) \
+		((be) ? ioread32be(KINETIS_PIT_PTR(base, reg)) \
+		      : ioread32(KINETIS_PIT_PTR(base, reg)))
+#define KINETIS_PIT_WR(be, base, reg, val) do { \
+		if (be) \

Quoted 6 lines · On Tuesday 30 June 2015 14:27:25 Paul Osmialowski wrote:

(quoted diff) +6 -0

+Example:
+
+aliases {
+	pit0 = &pit0;
+	pit1 = &pit1;
+	pit2 = &pit2;

Review

Quoted 10 lines

(quoted diff) +10 -0

+	pit3 = &pit3;
+};
+
+pit at 40037000 {
+	compatible = "fsl,kinetis-pit-timer";
+	reg = <0x40037000 0x100>;
+	clocks = <&mcg_pclk_gate 5 23>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;

All the subnodes seem to fall inside of the device's own register area, so I think it would be nicer to use a specific 'ranges' property that only translates the registers in question.

Quoted 12 lines

/ {

(quoted diff) +10 -0

+	aliases {
+		pit0 = &pit0;
+		pit1 = &pit1;
+		pit2 = &pit2;
+		pit3 = &pit3;
+	};
+
 	soc {
+		pit at 40037000 {
+			compatible = "fsl,kinetis-pit-timer";
+			reg = <0x40037000 0x100>;

Review

Quoted 10 lines

(quoted diff) +10 -0

+			clocks = <&mcg_pclk_gate 5 23>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			pit0: timer at 40037100 {
+				reg = <0x40037100 0x10>;
+				interrupts = <68>;
+				status = "disabled";
+			};

I don't think it's necessary to have both an alias and a label here. What do you use the alias for?

Review

Quoted 12 lines

(quoted diff) +12 -0

+
+#define KINETIS_PITMCR_PTR(base, reg) \
+	(&(((struct kinetis_pit_mcr_regs *)(base))->reg))
+#define KINETIS_PITMCR_RD(be, base, reg) \
+		((be) ? ioread32be(KINETIS_PITMCR_PTR(base, reg)) \
+		      : ioread32(KINETIS_PITMCR_PTR(base, reg)))
+#define KINETIS_PITMCR_WR(be, base, reg, val) do { \
+		if (be) \
+			iowrite32be((val), KINETIS_PITMCR_PTR(base, reg)); \
+		else \
+			iowrite32((val), KINETIS_PITMCR_PTR(base, reg)); \
+	} while (0)

These should really be written as inline functions. Can you explain why you need to deal with a big-endian version of this hardware? Can you configure the endianess of this register block and just set it to one of the two at boot time?

Quoted 7 lines

(quoted diff) +7 -0

+#define KINETIS_PIT_PTR(base, reg) \
+	(&(((struct kinetis_pit_channel_regs *)(base))->reg))
+#define KINETIS_PIT_RD(be, base, reg) \
+		((be) ? ioread32be(KINETIS_PIT_PTR(base, reg)) \
+		      : ioread32(KINETIS_PIT_PTR(base, reg)))
+#define KINETIS_PIT_WR(be, base, reg, val) do { \
+		if (be) \

Review

Quoted 10 lines

(quoted diff) +10 -0

+			iowrite32be((val), KINETIS_PIT_PTR(base, reg)); \
+		else \
+			iowrite32((val), KINETIS_PIT_PTR(base, reg)); \
+	} while (0)
+#define KINETIS_PIT_SET(be, base, reg, mask) \
+		KINETIS_PIT_WR(be, base, reg, \
+			KINETIS_PIT_RD(be, base, reg) | (mask))
+#define KINETIS_PIT_RESET(be, base, reg, mask) \
+		KINETIS_PIT_WR(be, base, reg, \
+			KINETIS_PIT_RD(be, base, reg) & (~(mask)))

Functions again. Also, just pass a pointer to your own data structure into the function, instead of the 'be' and 'base' values.

The 'set' and 'reset' functions look like they need a spinlock to avoid races.

Arnd

Quoted 17 lines · On Tuesday 30 June 2015 14:27:25 Paul Osmialowski wrote:

/ {

arch/arm/boot/dts/kinetis.dtsi +15 -0

+     aliases {
+             pit0 = &pit0;
+             pit1 = &pit1;
+             pit2 = &pit2;
+             pit3 = &pit3;
+     };
+
      soc {
+             pit at 40037000 {
+                     compatible = "fsl,kinetis-pit-timer";
+                     reg = <0x40037000 0x100>;
+                     clocks = <&mcg_pclk_gate 5 23>;
+                     #address-cells = <1>;
+                     #size-cells = <1>;
+                     ranges;
+

arch/arm/boot/dts/kinetis.dtsi line 28

Quoted 5 lines

arch/arm/boot/dts/kinetis.dtsi +5 -0

+                     pit0: timer at 40037100 {
+                             reg = <0x40037100 0x10>;
+                             interrupts = <68>;
+                             status = "disabled";
+                     };

I don't think it's necessary to have both an alias and a label here. What do you use the alias for?

drivers/clocksource/timer-kinetis.c new file +13 -0

+static struct kinetis_clock_event_ddata
+		kinetis_clockevent_tmrs[KINETIS_PIT_CHANNELS] = {
+	{
+		.evtdev = {
+			.name		= "kinetis-clockevent0",
+			.rating		= 200,
+			.features	=
+			    CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+			.set_next_event	= kinetis_clockevent_tmr_set_next_event,
+			.set_state_periodic =
+				kinetis_clockevent_tmr_set_state_periodic,
+			.set_state_oneshot =
+				kinetis_clockevent_tmr_set_state_oneshot,

drivers/clocksource/timer-kinetis.c line 296

Quoted 6 lines

drivers/clocksource/timer-kinetis.c new file +6 -0

+		clockevents_register_device(
+				&kinetis_clockevent_tmrs[chan].evtdev);
+
+		kinetis_pit_init(&kinetis_clockevent_tmrs[chan],
+						(rate / HZ) - 1);
+		kinetis_pit_enable(&kinetis_clockevent_tmrs[chan], 1);

No point doing this. The core code has invoked the set_periodic call back via clockevents_register_device() already.

Hi Thomas,

Quoted 10 lines · Thomas Gleixner

drivers/clocksource/timer-kinetis.c line 296

Quoted 6 lines

drivers/clocksource/timer-kinetis.c new file +6 -0

+		clockevents_register_device(
+				&kinetis_clockevent_tmrs[chan].evtdev);
+
+		kinetis_pit_init(&kinetis_clockevent_tmrs[chan],
+						(rate / HZ) - 1);
+		kinetis_pit_enable(&kinetis_clockevent_tmrs[chan], 1);

No point doing this. The core code has invoked the set_periodic call back via clockevents_register_device() already.

As I removed this kinetis_pit_enable() line, the timer did not start, therefore the system became unusable. What could be possible reason for that?

Quoted 3 lines

As I removed this kinetis_pit_enable() line, the timer did not start, therefore the system became unusable. What could be possible reason for that?

Well, you need to move both, the init and the enable into set_periodic().

- kinetis_pit_init(kinetis_tmr, (rate / HZ) - 1);

This is a very cheap and simple implementation of pinctrl driver for Kinetis SoC - its primary role is to provide means for enabling UART fuctionality on I/O PORT_E which will be utilized by the commits yet to come.

Signed-off-by Paul Osmialowski <[email protected]>

Surprisingly small amount of work was required in order to extend already existing eDMA driver with the support for Kinetis SoC architecture.

{

drivers/dma/fsl-edma.c +2 -3

+	struct device_node *np = pdev->dev.of_node;
+	int irq, errirq;
 	int ret;
 
-	fsl_edma->txirq = platform_get_irq_byname(pdev, "edma-tx");
-	if (fsl_edma->txirq < 0) {
-		dev_err(&pdev->dev, "Can't get edma-tx irq.\n");

drivers/dma/fsl-edma.c +0 -3

 	}
 
-	if (fsl_edma->txirq == fsl_edma->errirq) {
-		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
-				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);

drivers/dma/fsl-edma.c +45 -8

 		}
-	} else {
-		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
+
+		for (i = 0; i < (irqs - 1); i++) {
+			char irq_name[32];
+
+			sprintf(irq_name, "edma-tx-%d,%d", i, 16 + i);
+			irq = platform_get_irq_byname(pdev, irq_name);
+			if (irq < 0) {
+				dev_err(&pdev->dev, "Can't get %s irq.\n",
+							irq_name);
+				return irq;
+			}
+
+			ret = devm_request_irq(&pdev->dev, irq,
 				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
-		if (ret) {
-			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
-			return  ret;
+			if (ret) {
+				dev_err(&pdev->dev, "Can't register %s IRQ.\n",
+							irq_name);
+				return  ret;
+			}
 		}
 
-		ret = devm_request_irq(&pdev->dev, fsl_edma->errirq,
-				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
+		ret = devm_request_irq(&pdev->dev, errirq,
+			fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
 		if (ret) {
 			dev_err(&pdev->dev, "Can't register eDMA err IRQ.\n");
-			return  ret;
+		return  ret;
+		}
+	} else {
+		irq = platform_get_irq_byname(pdev, "edma-tx");
+		if (irq < 0) {
+			dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
+			return irq;
+		}
+
+		if (irq == errirq) {
+			ret = devm_request_irq(&pdev->dev, irq,
+				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+						"Can't register eDMA IRQ.\n");
+				return  ret;
+			}
+		} else {
+			ret = devm_request_irq(&pdev->dev, irq,
+				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+					    "Can't register eDMA tx IRQ.\n");
+				return  ret;
+			}

On Tue, Jun 30, 2015 at 02:27:27PM +0200, Paul Osmialowski wrote:
The patch title is not per subsystem semantics, pls fix that

Quoted 2 lines

Surprisingly small amount of work was required in order to extend already existing eDMA driver with the support for Kinetis SoC architecture.

And this doesn't tell me the stuff you added/removed/fixed in current driver to extend to Kinetis. Our changelog should tell what changed and why

Btw is this patch dependent upon the rest of the series?

drivers/dma/fsl-edma.c static void fsl_edma_free_chan_resources(struct dma_chan *chan) · line 792

Quoted 5 lines

static int

drivers/dma/fsl-edma.c +2 -1

-fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma)
+fsl_edma_irq_init(struct platform_device *pdev,
+		  struct fsl_edma_engine *fsl_edma)

please keep style changes in a separate patch

Quoted 8 lines

{

drivers/dma/fsl-edma.c +2 -3

+	struct device_node *np = pdev->dev.of_node;
+	int irq, errirq;
 	int ret;
 
-	fsl_edma->txirq = platform_get_irq_byname(pdev, "edma-tx");
-	if (fsl_edma->txirq < 0) {
-		dev_err(&pdev->dev, "Can't get edma-tx irq.\n");

drivers/dma/fsl-edma.c static void fsl_edma_free_chan_resources(struct dma_chan *chan) · line 801

Quoted 10 lines

drivers/dma/fsl-edma.c +3 -6

-		return fsl_edma->txirq;
-	}
-
-	fsl_edma->errirq = platform_get_irq_byname(pdev, "edma-err");
-	if (fsl_edma->errirq < 0) {
+	errirq = platform_get_irq_byname(pdev, "edma-err");
+	if (errirq < 0) {
 		dev_err(&pdev->dev, "Can't get edma-err irq.\n");
-		return fsl_edma->errirq;
+		return irq;

shouldn't this be errirq

Quoted 5 lines

drivers/dma/fsl-edma.c +0 -3

 	}
 
-	if (fsl_edma->txirq == fsl_edma->errirq) {
-		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
-				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);

drivers/dma/fsl-edma.c static void fsl_edma_free_chan_resources(struct dma_chan *chan) · line 810

Quoted 10 lines

drivers/dma/fsl-edma.c +7 -3

-		if (ret) {
-			dev_err(&pdev->dev, "Can't register eDMA IRQ.\n");
-			 return  ret;
+	if (fsl_edma->kinetis) {
+		int i;
+		int irqs = of_irq_count(np);
+
+		if (irqs <= 1) {
+			dev_err(&pdev->dev, "Wrong eDMA irq count %d\n", irqs);
+			return -EINVAL;

why not return irqs?

Quoted 59 lines

drivers/dma/fsl-edma.c +45 -8

 		}
-	} else {
-		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
+
+		for (i = 0; i < (irqs - 1); i++) {
+			char irq_name[32];
+
+			sprintf(irq_name, "edma-tx-%d,%d", i, 16 + i);
+			irq = platform_get_irq_byname(pdev, irq_name);
+			if (irq < 0) {
+				dev_err(&pdev->dev, "Can't get %s irq.\n",
+							irq_name);
+				return irq;
+			}
+
+			ret = devm_request_irq(&pdev->dev, irq,
 				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
-		if (ret) {
-			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
-			return  ret;
+			if (ret) {
+				dev_err(&pdev->dev, "Can't register %s IRQ.\n",
+							irq_name);
+				return  ret;
+			}
 		}
 
-		ret = devm_request_irq(&pdev->dev, fsl_edma->errirq,
-				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
+		ret = devm_request_irq(&pdev->dev, errirq,
+			fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
 		if (ret) {
 			dev_err(&pdev->dev, "Can't register eDMA err IRQ.\n");
-			return  ret;
+		return  ret;
+		}
+	} else {
+		irq = platform_get_irq_byname(pdev, "edma-tx");
+		if (irq < 0) {
+			dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
+			return irq;
+		}
+
+		if (irq == errirq) {
+			ret = devm_request_irq(&pdev->dev, irq,
+				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+						"Can't register eDMA IRQ.\n");
+				return  ret;
+			}
+		} else {
+			ret = devm_request_irq(&pdev->dev, irq,
+				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+					    "Can't register eDMA tx IRQ.\n");
+				return  ret;
+			}

drivers/dma/fsl-edma.c static void fsl_edma_free_chan_resources(struct dma_chan *chan) · line 871

Quoted 10 lines

drivers/dma/fsl-edma.c +8 -0

+
+			ret = devm_request_irq(&pdev->dev, errirq,
+				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+					    "Can't register eDMA err IRQ.\n");
+				return  ret;
+			}
 		}
 	}

Okay here we have a bunch of changes without explanation on why these are required and what are they supposed to do. So it is kind of very difficult to review!

So please split these up in sequence of patches where each patch does only ONE thing with right changelog messages

Thanks

Signature · 1 line

~Vinod

Note that <mach/memory.h> is needed (which is denoted by CONFIG_NEED_MACH_MEMORY_H) as it provides macros required for proper operation of DMA allocation functions.

Signed-off-by Paul Osmialowski <[email protected]>

---
 arch/arm/Kconfig                            |  4 ++
 arch/arm/boot/dts/kinetis.dtsi              | 34 ++++++++++++++++
 arch/arm/mach-kinetis/include/mach/memory.h | 61 +++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 arch/arm/mach-kinetis/include/mach/memory.h

arch/arm/Kconfig +0 -0

index b21592b..8ccffee 100644

arch/arm/Kconfig +7 -0

+
+#ifndef _MACH_KINETIS_MEMORY_H
+#define _MACH_KINETIS_MEMORY_H
+
+#ifndef __ASSEMBLY__
+
+/*

Quoted 3 lines · On Tuesday 30 June 2015 14:27:28 Paul Osmialowski wrote:

Note that <mach/memory.h> is needed (which is denoted by CONFIG_NEED_MACH_MEMORY_H) as it provides macros required for proper operation of DMA allocation functions.

You can't do this, it breaks compilation when multiple platforms are enabled.

Quoted 13 lines

Signed-off-by Paul Osmialowski <[email protected]>

---
 arch/arm/Kconfig                            |  4 ++
 arch/arm/boot/dts/kinetis.dtsi              | 34 ++++++++++++++++
 arch/arm/mach-kinetis/include/mach/memory.h | 61 +++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 arch/arm/mach-kinetis/include/mach/memory.h

arch/arm/Kconfig +0 -0

index b21592b..8ccffee 100644

arch/arm/Kconfig config ARCH_KINETIS · line 977

Quoted 7 lines

arch/arm/Kconfig +4 -0

@@ -971,6 +971,10 @@ config ARCH_KINETIS
 	select CLKSRC_KINETIS
 	select PINCTRL
 	select PINCTRL_KINETIS
+	select DMADEVICES
+	select FSL_EDMA
+	select DMA_OF
+	select NEED_MACH_MEMORY_H

I think DMA_OF is implied by dmaengine support in combination with CONFIG_OF

Quoted 7 lines

arch/arm/Kconfig +7 -0

+
+#ifndef _MACH_KINETIS_MEMORY_H
+#define _MACH_KINETIS_MEMORY_H
+
+#ifndef __ASSEMBLY__
+
+/*

arch/arm/Kconfig config ARCH_KINETIS

Quoted 10 lines

arch/arm/Kconfig +10 -0

+ * On Kinetis K70, consistent DMA memory resides in a special
+ * DDRAM alias region (non-cacheable DDRAM at 0x80000000).
+ *
+ */
+#define KINETIS_PHYS_DMA_OFFSET UL(0x80000000)
+
+/*
+ * Mask of the field used to distinguish DDRAM aliases
+ */
+#define KINETIS_DRAM_ALIAS_MASK UL(0xf8000000)

This should be expressed using the 'dma-ranges' properties in the bus nodes above any DMA master, the normal DMA mapping code will then do the right thing.

Arnd