Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sam0: Implement Linux style device tree clock control #15585

Closed
wants to merge 18 commits into from

Conversation

Sizurka
Copy link
Collaborator

@Sizurka Sizurka commented Apr 21, 2019

This series moves the clocking for SAMD2x to be entirely controlled by the device tree. This is similar to the ongoing work being done for #15363 but here the process is more invasive (it requires an clock provider definition) and the result is that the clock is controlled by the device tree too.

The primary motivation for the change was that the SAM0 peripherals tend to have relatively limited built in clock prescalers, but the SAM0 chip itself has a flexible clock generation system. So it makes sense to make the chip clocking available in a more generic way. For example, the SERCOM in I2C mode only has an 8-bit clock divider, so if using normal speed mode, this effectively limits the CPU frequency if it's clocked from the undivided core clock (e.g. SAMD5x). Similarly, the ADC has only a few powers of two divisors available, so achieving near its rated conversion rate means using an alternate clock than the CPU clock. In a case where at the least description is needed is the RTC (system timer) where SAMD2x can clock it at the full speed, SAMD5x is limited to a 32kHz clock.

Device Tree Update

The first change made is to describe the existing clock structure with the device tree. The clock definitions use the Linux bindings for clock control. Basically these define a clocks parent node with various nodes for each individual clock provider/oscillator. See here for an example.

For SAM0, there isn't a direct central clock controller like in the above example, instead each individual GCLK is defined separately and the peripherals reference the originating GCLK directly. This approach was chosen because each GCLK can have its own separate frequency, so specifying them all would get quite verbose.

So a sample of the SAM0 DTS ends up looking like:

clocks {
	dfll: dfll {
		compatible = "fixed-clock";
		clock-frequency = <48000000>;
		clocks = <&gclk1>;
		#clock-cells = <0>;
		label = "DFLL";
	};

	gclk0: gclk0 {
		compatible = "fixed-clock";
		clock-frequency = <48000000>;
		clocks = <&dfll>;
		#clock-cells = <0>;
		label = "GCLK_0";
	};
};
soc {
	sercom0: sercom@42000800 {
		compatible = "atmel,sam0-sercom";
		reg = <0x42000800 0x40>;
		status = "disabled";
		label = "SERCOM0";
		clocks = <&gclk0>;
	};
};

The above describes two clocks, the DFLL that provides a 48MHz "source" clock, with its own (not shown) reference clock input and GCLK0, which is what peripherals can actually reference directly. The SERCOM peripheral then uses GCLK0 as the clock input to the device.

This series incrementally converts SAM0 to use this kind of device tree. Initially the clock setup is only described with the device tree. Then peripherals are converted to use information from it, instead of simply assuming a constant setup. Finally, the SoC setup code is changed to actually allow for re-configuration of the clock tree based on the device tree.

Implementation

The first part of the implementation is simply describing the clock tree as it is used in the various boards. Since there are Kconfig options that change this somewhat, the initial setup assumes the board defaults. Thankfully, these Kconfig options have no peripheral visible effects, so there are no inconsistent behaviors.

Then a clock_control driver is implemented that can be used to query and manipulate peripheral GCLK connections. This can be used to replace the current hard coded GCLK enables that all peripherals use. It also has a frequency query that will be used to replace the current static defines that the peripherals use.

Next, peripherals are converted to use the clock control driver. Since the current clock setup simply describes the existing GCLK tree (i.e. nearly everything hangs off GCLK0), this has no current behavior change.

Finally, the SoC setup is re-written to use the DTS generated information to actually generate the clock tree. The resulting clock tree should be identical to the currently used one, since the DTS is set to describe it already. So the final result of the series is no functional changes, but much greater flexibility.

Dynamic Control

After this series, it becomes possible to control the clocks from DTS overlays to allow for more flexibility. As a toy example, the below changes the main SoC clock to use a 48MHz clock generated by the FDPLL from the calibrated 32kHz RC oscillator:

&osc32k {
	status = "ok";
};

&gclk1 {
	clocks = <&osc32k>;
	clock-frequency = <32768>;
};

&fdpll {
	status = "ok";
	clock-frequency = <48000000>;
	clocks = <&gclk1>;
};

&gclk0 {
	clocks = <&fdpll>;
};

This example first turns on the calibrated oscillator, then changes GCLK1 to use it as the input, and finally enables the FDPLL (the multiplier is calculated automatically) and tells GCLK0 to use it.

Future Work

During work on this series, one thing I noticed was that the manual specifies the maximum input clock for the DFLL in closed loop mode as 33 kHz (section 37.6, table 37-7, page 985). However the current default clock setup for any SAMDx board without a 32kHz crystal is to use the undivided 8MHz RC oscillator as the reference. This does appear to "work", but I'm not sure it's actually using the reference (i.e. it's probably operating in (un-calibrated) open loop mode with the "reference stopped" logic kicking in). This appears to have been carried over/cargo culted from various online forums, where, as far as I can tell, the initial recommendation of adding a divisor to GCLK1 before using it as the reference got lost at one point. So in the future the clock setup should either be changed to use a divided OSC8M, to simply use the internal 32kHz RC oscillator, or to simply operate in open loop mode explicitly (like it's probably doing already).

Another candidate for some clock related work is enabling USB clock recovery for the DFLL. This is Microchip's recommendation if using the chip for USB with no crystal present, but the current setups do not do that on the boards without crystals. That is, they simply operate in closed loop mode from the internal oscillator, which may not even work (above). So in the future, an option could be implemented to allow for operating in this mode.

These issues are not part of this series, since the goal of it was no functional changes.

Alternative Implementations

One possible alternative implementation uses clock_control drivers for all possible clocks on the SoC and relies on the init priority to handle sequencing. This has the advantage of abstracting the clocks entirely, but ends up being rather hard to follow. Additionally, most of the clocks (i.e. anything other than a GCLK) is basically unused after SoC startup, since the peripherals do not interact with them directly. I have an example of this kind of implementation at Sizurka#3.

Another possibility is to enhance the DTS parsing to extract information about the parent clocks directly. This can allow the needed information to configure the clocks at compile time, but could be limited in some cases (e.g. if a device needed to know about the source of its source clock it becomes awkward quickly). For strictly SAM0 devices, I tend to favor this approach, but it's also more implementation specific, so the above is my attempt at a best of both worlds approach (e.g. clock setup is mostly compile time calculated but peripherals use the device abstraction). My original implementation of this is at Sizurka#4.

One notable disadvantage of the device based approaches is that the clock frequency is unavailable at compile time. So any checks/asserts on it have to be runtime where they may be missed. Additionally this complicates counter drivers, which require their frequency to be available at compile time. So for those, it will be necessary to assume that the requested frequency is correct, then calculate the actual frequency at run time and warn if the two don't match.

@pizi-nordic
Copy link
Collaborator

pizi-nordic commented Apr 25, 2019

Thank you for this PR! We would like to introduce clock nodes in the same way as you presented in order to trace dependencies between peripherals and clock sources. However such approach will need a strong cooperation of the parties maintaining different SoCs - we will have to update DTS files, DTS scripts and probably drivers with their infrastructure. Could we raise this on the dev-review meeting?

@pizi-nordic pizi-nordic added the dev-review To be discussed in dev-review meeting label Apr 25, 2019
@benpicco benpicco added the platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) label May 2, 2019
@benpicco benpicco mentioned this pull request May 9, 2019
15 tasks
@nashif nashif removed the dev-review To be discussed in dev-review meeting label May 9, 2019
@benpicco
Copy link
Collaborator

benpicco commented May 9, 2019

Again, nice work!
This squashed one remaining wart with sam0 and just as I was stumbling upon it, I figured you already did propose a solution for it.

@zephyrbot
Copy link
Collaborator

zephyrbot commented May 11, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@Sizurka Sizurka changed the title [RFC] soc: sam0: Implement SAMD2x device tree clocking sam0: Implement Linux style device tree clock control May 12, 2019
@Sizurka Sizurka marked this pull request as ready for review May 12, 2019 13:01
@Sizurka Sizurka force-pushed the sam0-dts-clock branch 2 times, most recently from f341354 to ea153d0 Compare May 20, 2019 13:03
Add clock-output-names as an alternative to label for the clock
controller extraction.

Signed-off-by: Derek Hageman <[email protected]>
Create a common base for a clock provider binding, in the spirit of
the base binding used for all devices.

This also replaces the "label" property to "clock-output-names"
to be closer to the Linux binding.  Since nothing currently uses
the property, no other changes are required.

Signed-off-by: Derek Hageman <[email protected]>
Add Linux style clock information to the DTS for SAM0 SoCs.
The current setup simply describes the clock tree as it is
configured by the static initialization code in the default
setup.

Signed-off-by: Derek Hageman <[email protected]>
Add clock definitions to the DTS overlays for the various SAM0
boards.  This only reflects the current default clock setup
for these boards.

Signed-off-by: Derek Hageman <[email protected]>
Add a driver to interface with the SAM0 GCLK tree.  This does not
handle any of the initialization and is intended to be used
simply by peripherals to enable their GCLK source.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Convert explicit GCLK usage to the generic clock_control
based driver.

Signed-off-by: Derek Hageman <[email protected]>
Change the clock setup for SAMD2x to be controlled by the device
tree definitions of the clocks.  The clock label comparisons
make use of compiler builtins that optimize to simple constants.

Signed-off-by: Derek Hageman <[email protected]>
The clock control is now fully defined by the device tree, so
remove the Kconfig options associated with it.

Signed-off-by: Derek Hageman <[email protected]>
Add myself as code owner for the SAM0 clock_control driver.

Signed-off-by: Derek Hageman <[email protected]>
@Sizurka
Copy link
Collaborator Author

Sizurka commented Jul 17, 2019

I don't think assigned-clock-rates is a good fit here. The Linux binding description of that is for a node that configures a parent clock and is the only consumer of that clock. Here the clocks are either internal to the node or exist as one to many clocks so they can have multiple consumers.

Basically, my usage for clock-frequency in nodes was to calculate divisors and/or multipliers required for that node to produce the requested clock rate. For the clock nodes (/clocks/* ) it is also the output frequency of the clock. For device nodes (/soc/* and /cpus/cpu0) it is used with the input clock frequency to calculate the divisor required to produce the clock rate.

The ADC and TC32 (counter) have two asynchronous clocks involved: the input/register/device clock (the clocks property) and a device internal clock derived from the input clock. The internal clock frequency is set with the nodes clock-frequency and the driver calculates the divisor from the input clock required to achieve that frequency. The internal clock cannot be directly used outside of the device (it cannot act as a clock provider), but I'm not aware of a better "common" property name to reflect this.

The clock nodes all use clock-frequency as expected: it's the output frequency of the clock. The SoC init code uses this frequency and the input frequency (if any) to calculate the required settings for the clock hardware.

For example, the branch of the clock tree driving the ADC might look like: XOSC32K -> GCLK1 -> DFLL -> GCLK4 -> ADC0

DT node Clock Frequency Zephyr Subsystem Calculations Purpose
/clocks/xosc32k 32768 (fixed) SoC init Hardware clock source (one to many GCLK and other clocks)
/clocks/gclk1 32768 (selectable <= input) clock_control Divisor = 1 (32768/32768) Clock distribution (one to many device)
/clocks/dfll 48000000 (fixed, with reference) SoC Init Multiplier = 1464 (48000000 / 32768) Hardware clock source (one to many GCLK)
/clocks/gclk4 16000000 (selectable <= input) clock_control Divisor = 3 (48000000 / 16000000) Clock distribution (one to many device)
/soc/adc 16000000 - async (fixed = input) adc Device clock (registers, internal)
/soc/adc 2000000 - sample (selectable <= input) adc Divisor = 8 (16000000 / 2000000) Sampling clock (acquisition time)

Here there is an external 32KiHz oscillator (/clocks/xosc32k) which is connected to GCLK1 (/clocks/gclk1). GCLK1 is then used as the reference frequency for the DFLL (/clocks/dfll), which it uses to adjust its calibration dynamically. Then the DFLL is connected to GCLK4 (/clocks/gclk4), but with GCLK4 providing a divisor (/3) so that the frequency exposed on GCLK4 is 16MHz. Finally the ADC (/soc/adc) gets its "asynchronous" clock from GCLK4 (the clocks property), this clock is used for register access and internal device clocking.
The ADC then has a second clock used to control the sampling hardware (i.e. the rate at which the SAR hardware is clocked). This clock rate is derived from the asynchronous clock with a divisor calculated from the ADC's clock-frequency property. So here the clock-frequency property controls an entirely internal clock to the device/node.

Overall, I'm not against using a different property for the device internal clock rates, but the upstream description of assigned-clock-rates doesn't fit. Most (all?) of the examples of similar upstream usage I found used vendor specific properties for this (e.g. st,adc-freq, fsl,adck-max-frequency, etc).

Unfortunately, it doesn't look like the current DTS parser can handle the comma in property names (I haven't tracked down why), so even if changed, it would still end be being somewhat different. I can certainly change it for the device nodes, but I do think clock-frequency fits the clocks nodes in this case.

So, would changing the ADC and counter frequency properties to (e.g.) atmel-sam0-adc-frequency and atmel-sam0-tc32-frequency be sufficient?

@galak
Copy link
Collaborator

galak commented Jul 19, 2019

Let me chat with the Linux clock maintainers to see how we should be representing things. I have a feeling the wording around assigned-clock-rates is a bit confusing.

@galak
Copy link
Collaborator

galak commented Jul 22, 2019

@Sizurka FYI:

In talking to the Linux clock maintainers I think we should have the DTS look something like:

(Note, just show GCLK and DFLL in this example)

                dfll: dfll {
                        compatible = "atmel,sam0-dfll";
                        assigned-clock-rates = <48000000>;
                        assigned-clock-parent = <&gclk 1>;
                        assigned-clocks = <&gclk 1>;
                        clocks = <&gclk 0>, <&gclk 1>, <&gclk 2>, <&gclk 3>,
                                 <&gclk 4>; <&gclk 5>, <&gclk 6>, <&gclk 7>;
                        #clock-cells = <0>;
                        clock-output-names = "DFLL";
                };

                gclk: gclk {
                        compatible = "atmel,sam0-gclk";
                        reg = <0x40000C00 0xc>;
                        clocks = <&osc32k &ulposc32k &osc8m &xosc32k &xosc>;
                        #clock-cells = <1>;
                        clock-output-names = "GCLK_0", "GCLK_1", "GCLK_2",
                                             "GCLK_3", "GCLK_4", "GCLK_5",
                                             "GCLK_6", "GCLK_7", "GCLK_8";
                };

In the example, we are setting dfll to gclk1 with a freq of 48000000. gclk isn't assigned to which clock source in this snippet.

@galak galak removed the dev-review To be discussed in dev-review meeting label Jul 25, 2019
@finikorg finikorg removed their request for review August 30, 2019 08:21
@galak galak added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Jan 30, 2020
@jfischer-no jfischer-no removed their request for review March 4, 2020 09:43
@nandojve nandojve mentioned this pull request Apr 25, 2020
61 tasks
@ioannisg
Copy link
Member

@Sizurka is this a stale PR?

@ioannisg ioannisg removed their request for review April 28, 2020 18:09
@Sizurka
Copy link
Collaborator Author

Sizurka commented Apr 28, 2020

Yes, I was unable to come up with a way to actually generate the clock control from the requested DT structure; I'll close the PR.

@Sizurka Sizurka closed this Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ADC Analog-to-Digital Converter (ADC) area: Boards area: Clock Control area: Counter area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: I2C area: SPI SPI bus area: Timer Timer area: Watchdog Watchdog platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants