Re: [PATCH v3 17/19] clk: at91: add PMC clk device tree binding doc.

From: Nicolas Ferre
Date: Tue Oct 08 2013 - 08:42:25 EST

On 08/10/2013 14:37, boris brezillon :
On 08/10/2013 11:44, Nicolas Ferre wrote:
On 08/08/2013 09:19, Boris BREZILLON :
This patch adds new at91 clks dt bindings documentation.

Signed-off-by: Boris BREZILLON <b.brezillon@xxxxxxxxxxx>
---
.../devicetree/bindings/clock/at91-clock.txt | 312
++++++++++++++++++++
1 file changed, 312 insertions(+)
create mode 100644
Documentation/devicetree/bindings/clock/at91-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/at91-clock.txt
b/Documentation/devicetree/bindings/clock/at91-clock.txt
new file mode 100644
index 0000000..04739da
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/at91-clock.txt
@@ -0,0 +1,312 @@
+Device Tree Clock bindings for arch-at91
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of the following:
+ "atmel,at91rm9200-pmc" or
+ "atmel,at91sam9g45-pmc" or
+ "atmel,at91sam9n12-pmc" or
+ "atmel,at91sam9x5-pmc" or
+ "atmel,at91sam9g35-pmc" or

Already said in previous patches: 9g35 is not different from the 9x5:
it was a bug in the older datasheet.
I'll drop it.

+ "atmel,sama5d3-pmc":
+ at91 PMC (Power Management Controller)
+ All at91 specific clocks (clocks defined below) must be child
+ node of the PMC node.
+
+ "atmel,at91rm9200-clk-main":
+ at91 main oscillator
+
+ "atmel,at91rm9200-clk-master" or
+ "atmel,at91sam9x5-clk-master":
+ at91 master clock
+
+ "atmel,at91sam9x5-clk-peripheral" or
+ "atmel,at91rm9200-clk-peripheral":
+ at91 peripheral clocks
+
+ "atmel,at91rm9200-clk-pll" or
+ "atmel,at91sam9g45-clk-pll" or
+ "atmel,at91sam9g20-clk-pllb" or
+ "atmel,sama5d3-clk-pll":
+ at91 pll clocks
+
+ "atmel,at91sam9x5-clk-plldiv":
+ at91 plla divisor
+
+ "atmel,at91rm9200-clk-programmable" or
+ "atmel,at91sam9g45-clk-programmable" or
+ "atmel,at91sam9x5-clk-programmable":
+ at91 programmable clocks
+
+ "atmel,at91sam9x5-clk-smd":
+ at91 SMD (Soft Modem) clock
+
+ "atmel,at91rm9200-clk-system":
+ at91 system clocks
+
+ "atmel,at91rm9200-clk-usb" or
+ "atmel,at91sam9x5-clk-usb":
+ at91 usb clock
+
+ "atmel,at91sam9x5-clk-utmi":
+ at91 utmi clock
+
+Required properties for PMC node:
+- reg : defines the IO memory reserved for the PMC.
+- interrupts : shall be set to PMC interrupt line.
+- interrupt-controller : tell that the PMC is an interrupt controller
+- #interrupt-cells : must be set to 2. The first cell encodes the
interrupt id

Please add more information about these values.

The first cell encodes the clk/interrupt id, which is represented by the
bit position in the PMC_SR register:
- MAIN clk = 0
- PLLA = 1
- ...

+ the second cell encodes the interrupt type.

Here also: is it always the same type that shall be given? Following
which rule? What are the allowed values?

Yes it's always IRQ_TYPE_LEVEL_HIGH, maybe I should just define one cell
and drop the irq type cell.

Yes. I am in favor for what you propose.



+For example:
+ pmc: pmc@fffffc00 {
+ compatible = "atmel,sama5d3-pmc";
+ interrupts = <AT91_ID_SYS IRQ_TYPE_LEVEL_HIGH 7>;

It is an habit not to use macro names in device tree examples (even if
it is true that it is more readable).

I'll use numerical values instead of macros (anyway, the AT91_ID_XX will
be dropped).


+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ /* put at91 clocks here */
+ };
+
+Required properties for main clock:
+- interrupt-parent : must reference the PMC node.
+- interrupts : shall be set to "<AT91_PMC_MOSCS IRQ_TYPE_LEVEL_HIGH>".

Ditto. Here you can use the numerical value and also specify the macro
name. But the numerical value should prevail.
Okay


+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks (optional if clock-frequency is provided) : shall be the
slow clock
+ phandle. This clock is used to compute the main clock rate if
+ "clock-frequency" is not provided.
+- clock-frequency: the main oscillator frequency.Prefer the use of

Nit. one white space missing

+ "clock-frequency" over automatic clock rate computation.


+
+For example:
+ main: mainck {
+ compatible = "atmel,at91rm9200-clk-main";
+ interrupt-parent = <&pmc>;
+ interrupts = <AT91_PMC_MOSCS IRQ_TYPE_LEVEL_HIGH>;

Ditto

+ #clock-cells = <0>;
+ clocks = <&ck32k>;
+ clock-frequency = <18432000>;
+ };
+
+Required properties for master clock:
+- interrupt-parent : must reference the PMC node.
+- interrupts : shall be set to "<AT91_PMC_MCKRDY IRQ_TYPE_LEVEL_HIGH>".

Ditto

+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : shall be the master clock sources (see atmel datasheet)
phandles.
+ e.g. "<&ck32k>, <&main>, <&plla>, <&pllb>".
+- atmel,clk-output-range : minimum and maximum clock frequency (two u32
+ fields).
+ e.g. output = <0 133000000>; <=> 0 to 133MHz.
+- atmel,clk-divisors : master clock divisors table (four u32 fields).
+ 0 <=> reserved value.
+ e.g. divisors = <1 2 4 6>;
+- atmel,master-clk-have-div3-pres : some SoC use the reserved value
7 in the
+ PRES field as CLOCK_DIV3 (e.g sam9x5).

I will check with care the master clock driver as this one is pretty
picky about changes that could affect it! Note that in previous clock
implementation we did not touched the MCK configuration, we were only
reading it...

Anyway, let's keep this binding but make sure that driver is written
with extreme care ;-)

+
+For example:
+ mck: mck {
+ compatible = "atmel,at91rm9200-clk-master";
+ interrupt-parent = <&pmc>;
+ interrupts = <AT91_PMC_MCKRDY IRQ_TYPE_LEVEL_HIGH>;
+ #clock-cells = <0>;
+ atmel,clk-output-range = <0 133000000>;
+ atmel,clk-divisors = <1 2 4 0>;
+ };
+
+Required properties for peripheral clocks:
+- #clock-cells : from common clock binding; shall be set to 1. The
second cell
+ is used to encode the peripheral id. Peripheral ids are defined in
+ atmel's SoC datasheets.
+- clocks : shall be the master clock phandle.
+ e.g. clocks = <&mck>;
+- name: device tree node describing a specific system clock.
+ * atmel,clk-id: peripheral id. Peripheral id macros should be used.

No. Please use raw numbers. We will not switch to macros for these
peripheral IDs.
Sure, I'll change this.

+ * atmel,clk-default-divisor (optional, only available for
+ "atmel,at91sam9x5-clk-peripheral"): sam9x5 and sama5d3 SoC
provides
+ configurable peripheral clock divisor. If you define this
property
+ (u32), the default divisor will be applied when enabling
+ peripheral clock. If not provided the peripheral clock is not
+ divided.
+
+For example:
+ periph: periphck {
+ compatible = "atmel,at91sam9x5-clk-peripheral";
+ #clock-cells = <1>;
+ clocks = <&mck>;
+
+ pioA_clk {
+ atmel,clk-id = <AT91SAM9X5_ID_PIOA>;

Ditto.

+ atmel,clk-default-divisor = <1>;
+ };
+
+ pioB_clk {
+ atmel,clk-id = <AT91SAM9X5_ID_PIOB>;
+ atmel,clk-default-divisor = <2>;
+ };
+ };
+
+
+Required properties for pll clocks:
+- interrupt-parent : must reference the PMC node.
+- interrupts : shall be set to "<AT91_PMC_LOCKA IRQ_TYPE_LEVEL_HIGH>".

Ditto

+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : shall be the main clock phandle.
+- atmel,clk-id : pll id. PLL id macros should be used.

No macros here, simply raw numbers. So this mean that we must have
some documentation of these numbers.

+- atmel,clk-input-range : minimum and maximum source clock frequency
(two u32
+ fields).
+ e.g. input = <1 32000000>; <=> 1 to 32MHz.
+- #atmel,pll-clk-output-range-cells : number of cells reserved for
pll output
+ range description. Sould be set to 2, 3
+ or 4.
+ * 1st and 2nd cells represent the frequency range (min-max).
+ * 3rd cell is optional and represents the OUT field value for
the given
+ range.
+ * 4th cell is optional and represents the ICPLL field (PLLICPR
+ register)
+- atmel,pll-clk-output-ranges : pll output frequency ranges +
optional parameter
+ depending on #atmel,pll-output-range-cells
+ property value.
+
+For example:
+ plla: pllack {
+ compatible = "atmel,at91sam9g45-clk-pll";
+ interrupt-parent = <&pmc>;
+ interrupts = <AT91_PMC_LOCKA IRQ_TYPE_LEVEL_HIGH>;
+ #clock-cells = <0>;
+ clocks = <&main>;
+ atmel,clk-id = <AT91_PLLA_CLK>;
+ atmel,clk-input-range = <2000000 32000000>;
+ #atmel,pll-clk-output-range-cells = <4>;
+ atmel,pll-clk-output-ranges = <74500000 800000000 0 0
+ 69500000 750000000 1 0
+ 64500000 700000000 2 0
+ 59500000 650000000 3 0
+ 54500000 600000000 0 1
+ 49500000 550000000 1 1
+ 44500000 500000000 2 1
+ 40000000 450000000 3 1>;
+ };
+
+Required properties for plldiv clocks:
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : shall be the plla clock phandle.
+
+For example:
+ plladiv: plladivck {
+ compatible = "atmel,at91sam9x5-clk-plldiv";
+ #clock-cells = <0>;
+ clocks = <&plla>;
+ };

Maybe a little explanation about this clock. It is a fixed divided
(how many times?) clock issued from the specified clock.

It is a fixed clock divider (by 2) and AFAIK clk parent must be plla.
I'll add more precision about this clk.


+
+Required properties for programmable clocks:
+- interrupt-parent : must reference the PMC node.
+- #clock-cells : from common clock binding; shall be set to 1. The
second cell
+ is used to encode the programmable clock id.
+ Peripheral ids are in atmel's SoC
+ datasheets.
+- clocks : shall be the programmable clock source phandles.
+ e.g. clocks = <&clk32k>, <&main>, <&plla>, <&pllb>;
+- name: device tree node describing a specific prog clock.
+ * atmel,clk-id : programmable clock id (register offset from PCKx
+ register).
+ * interrupts : shall be set to
+ "<AT91_PMC_PCKRDY(id) IRQ_TYPE_LEVEL_HIGH>".

Ditto

+
+For example:
+ prog: progck {
+ compatible = "atmel,at91sam9g45-clk-programmable";
+ interrupt-parent = <&pmc>;
+ #clock-cells = <1>;
+ clocks = <&clk32k>, <&main>, <&plladiv>, <&utmi>, <&mck>;
+
+ prog0 {
+ atmel,clk-id = <0>;
+ interrupts = <AT91_PMC_PCKRDY(0) IRQ_TYPE_LEVEL_HIGH>;

Ditto

+ };
+
+ prog1 {
+ atmel,clk-id = <1>;
+ interrupts = <AT91_PMC_PCKRDY(1) IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
+
+
+Required properties for smd clock:
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : shall be the smd clock source phandles.
+ e.g. clocks = <&plladiv>, <&utmi>;
+
+For example:
+ smd: smdck {
+ compatible = "atmel,at91sam9x5-clk-smd";
+ #clock-cells = <0>;
+ clocks = <&plladiv>, <&utmi>;
+ };
+
+Required properties for system clocks:
+- #clock-cells : from common clock binding; shall be set to 1. The
second cell
+ is used to encode the system clock id (bit used in SCER/SCDR
register).
+- name: device tree node describing a specific system clock.
+ * id: system clock id (bit position in SCER/SCDR/SCSR registers).
+ System clock id macros should be used.

Ditto

+
+For example:
+ system: systemck {
+ compatible = "atmel,at91rm9200-clk-system";
+ #clock-cells = <1>;
+
+ ddrck {
+ atmel,clk-id = <AT91_DDRCK_SYS_CLK>;
+ };
+
+ uhpck {
+ atmel,clk-id = <AT91_UHP_SYS_CLK>;
+ };
+
+ udpck {
+ atmel,clk-id = <AT91_UDP_SYS_CLK>;
+ };
+ };
+
+
+Required properties for usb clock:
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : shall be the smd clock source phandles.
+ e.g. clocks = <&pllb>;
+- atmel,clk-divisors (only available for "atmel,at91rm9200-clk-usb"):
+ usb clock divisor table.
+ e.g. divisors = <1 2 4 0>;
+- atmel,usb-clk-src0-unused (only available for
"atmel,at91sam9x5-clk-usb"):
+ Some SoC (sam9n12) use usb source 0 to disable the usb clock.

I am not sure that we should use a special case for this device.
The enable/disable is still done by system clock register set.

It is true that this bit is defined differently but just because the
only source for this clock is pllb.

Yes, but at least, I need to know that I must add one to the clock
parent id when I set the USBS field in the PMC_USB register.
Or I can drop the atmel,usb-clk-src0-unused property and just add a new
"atmel,at91sam9n12-clk-usb" compatible string,
which might be a better solution here.

Tell me what you'd prefer.

Yep, that is what I was thinking about: use a specific "atmel,at91sam9n12-clk-usb" compatible string.


+
+For example:
+ usb: usbck {
+ compatible = "atmel,at91sam9x5-clk-usb";
+ #clock-cells = <0>;
+ clocks = <&plladiv>, <&utmi>;
+ };
+
+ usb: usbck {
+ compatible = "atmel,at91rm9200-clk-usb";
+ #clock-cells = <0>;
+ clocks = <&pllb>;
+ atmel,clk-divisors = <1 2 4 0>;
+ };
+
+
+Required properties for utmi clock:
+- interrupt-parent : must reference the PMC node.
+- interrupts : shall be set to "<AT91_PMC_LOCKU IRQ_TYPE_LEVEL_HIGH>".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : shall be the main clock source phandle.
+
+For example:
+ utmi: utmick {
+ compatible = "atmel,at91sam9x5-clk-utmi";
+ interrupt-parent = <&pmc>;
+ interrupts = <AT91_PMC_LOCKU IRQ_TYPE_LEVEL_HIGH>;
+ #clock-cells = <0>;
+ clocks = <&main>;
+ };


Many things in this patch that will have incident in driver code. I
will try to be coherent when I review the driver patches ;-)

Anyway, all this seem good!

Bye,

Thanks for the detailed review.






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